Skip to content

Conversation

axelboc
Copy link
Collaborator

@axelboc axelboc commented Aug 9, 2024

Fix #1273

See Vite's documentation to learn about this modern front-end bundler.

I've kept the build folder the same (ui/build) but I did change the front-end port to 5173, which is Vite's default port. You'll note that both the dev and build are much, much faster thanks to esbuild — the "UI" CI workflow is more than one minute faster.

@axelboc axelboc force-pushed the vite branch 5 times, most recently from 500780b to 9766223 Compare August 9, 2024 13:46
@axelboc
Copy link
Collaborator Author

axelboc commented Aug 9, 2024

The three-click centring E2E test still seems to be failing after mounting the sample (the "Current" tab doesn't change to "Sample: test - test"). I have a feeling it's linked to the fact that socket.io switches to polling transport mode during Cypress tests for some reason I can't figure out...

I've skipped it for now, since it seems to work outside of Cypress.

@axelboc axelboc marked this pull request as ready for review August 9, 2024 13:52
});

it('Each click is rotating the sample by 90 degrees', () => {
it.skip('Each click is rotating the sample by 90 degrees', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping this test for now, as explained, but if you have any clue @walesch-yan, please let me know 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I have no direct answer to this behavior. My guess would be that some configuration mismatch on cypress' side leads to networking issues. This definitely needs to be investigated in more detail.

Copy link
Contributor

@fabcor-maxiv fabcor-maxiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always give thumbs up for cleaning up, removing things, and modernizing : )

@marcus-oscarsson
Copy link
Member

marcus-oscarsson commented Aug 12, 2024

Very nice thanks for the excellent work :)

I don't mind using a browser extension for the redux store debug. I guess its possible to add a variable for it if its absolutely necessary, @meguiraun and @elmjag ? Maybe in that case it would be nice to have the option to have no logging at all. But a question out of curiosity, how do you do to update browser version and so on if you don't have access to internet ?

I'm happy to merge this :), there is a conflict from a previously merged PR, maybe you could have a look @axelboc

@fabcor-maxiv
Copy link
Contributor

how do you do to update browser version and so on if you don't have access to internet ?

As far as I understand we have our own proxies/mirrors for the system package repositories of the Linux distribution.

@marcus-oscarsson
Copy link
Member

how do you do to update browser version and so on if you don't have access to internet ?

As far as I understand we have our own proxies/mirrors for the system package repositories of the Linux distribution.

I see,

I'll go a head and merge this later today and we can sort out any remaining issues with how to handle the redux logging in a future PR, let me know if you think this approach is too progressive or if you have other concerns.

@axelboc
Copy link
Collaborator Author

axelboc commented Aug 13, 2024

I've added an environment variable called VITE_REDUX_LOGGER_ENABLED and documented how to go about overriding it: 6f678d7

@marcus-oscarsson
Copy link
Member

Thanks !

@fabcor-maxiv
Copy link
Contributor

I am thinking maybe this needs some documentation...

If things have changed for developer experience in a non-backwards-compatible way -- for example the change of port number, removal of redux-logger (assuming we go through with this part) -- then it might be worth an announcement on the mailing list.

And then it might be worth documenting (in /docs) that we have redux-devtools and that it needs a browser extension (again assuming we go through with this).

@marcus-oscarsson
Copy link
Member

@fabcor-maxiv: The last commit also added the possibility to use redux-logger and it was documented as well

https://github.com/mxcube/mxcubeweb/pull/1335/files#diff-425da2324e83ab4adea4825089d26a6b2b0d0dc15cbd54a452499c1f4445909f

The missing part is perhaps the port change from 3000 to 5173 ?

@fabcor-maxiv
Copy link
Contributor

fabcor-maxiv commented Aug 13, 2024

The last commit also added the possibility to use redux-logger and it was documented as well

Oh, good, I had not seen this had been added : )

I am not sure, though, can I use this VITE_REDUX_LOGGER_ENABLED environment variable at run-time or is it only for build-time (and hot-reload server)? Well, it can't possibly be at run-time, can it?

@axelboc
Copy link
Collaborator Author

axelboc commented Aug 13, 2024

Well, it can't possibly be at run-time, can it?

Environment variables are read when the dev server starts (pnpm --prefix ui start) and the project is built (pnpm --prefix ui build).

@axelboc
Copy link
Collaborator Author

axelboc commented Aug 13, 2024

If things have changed for developer experience in a non-backwards-compatible way -- for example the change of port number, removal of redux-logger (assuming we go through with this part) -- then it might be worth an announcement on the mailing list.

I can revert the port change if it's easier?

@axelboc
Copy link
Collaborator Author

axelboc commented Aug 13, 2024

And then it might be worth documenting (in /docs) that we have redux-devtools and that it needs a browser extension (again assuming we go through with this).

Good idea, I can add a section about the devtools.

@fabcor-maxiv
Copy link
Contributor

fabcor-maxiv commented Aug 13, 2024

If things have changed for developer experience in a non-backwards-compatible way -- for example the change of port number, removal of redux-logger (assuming we go through with this part) -- then it might be worth an announcement on the mailing list.

I can revert the port change if it's easier?

I'd rather we leave things to their default values. But maybe some people will be surprised by the change. That is why I think an announcement on the dev mailing list would make sense.

And/or there could be a line in our documentation explaining how to set the port to 3000. But I assume it would be no more than a copy-paste of Vite's documentation and a link back to Vite's documentation, which I find slightly pointless to have (and maintain) in our own documentation pages. So that would not be my preferred solution.


Independent from all that, can we have a paragraph in https://mxcubeweb.readthedocs.io/en/latest/dev/front-end.html that says that we have Vite as the builder (with link to their home page)?

@axelboc
Copy link
Collaborator Author

axelboc commented Aug 13, 2024

Independent from all that, can we have a paragraph in https://mxcubeweb.readthedocs.io/en/latest/dev/front-end.html that says that we have Vite as the builder (with link to their home page)?

Yep, working on it 👍

@axelboc
Copy link
Collaborator Author

axelboc commented Aug 13, 2024

I took the opportunity to expand the front-end documentation: https://github.com/mxcube/mxcubeweb/pull/1335/files#diff-425da2324e83ab4adea4825089d26a6b2b0d0dc15cbd54a452499c1f4445909f

@marcus-oscarsson
Copy link
Member

Nice, also good with the best practices for functional components and hooks connecting to the store 👍

@marcus-oscarsson marcus-oscarsson merged commit 01c6491 into develop Aug 13, 2024
@marcus-oscarsson marcus-oscarsson deleted the vite branch August 13, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Create React App with Vite
6 participants