-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add opt-in bundling with Vite #1262
base: development
Are you sure you want to change the base?
Add opt-in bundling with Vite #1262
Conversation
I gave the vite dev mode a spin, and its blazing fast!!! The QOL updates are unreal with Vite's Hot Module Reload. Getting client side updates without having to wait for webpack to re-bundle, then having to refresh the page over and over again feels amazing! Looking forward to this PR clearing. Thanks for working on this! |
A few points on this where I have thoughts/questions. I may have the ports/usage wrong so am confused.
These are notes that are for the record/future:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to @hyperupcall for doing another PR to help with OED technology. I made a comment on the PR with some thoughts/questions that I would like to understand. Other than that, the PR works as expected and the code looks good.
I tried running this some more and note this:
I was talking with @ChrisMart21 and they are not seeing this. Thus, I wonder if I am doing something wrong or there is something about my setup. The only change I made from the PR is to change the three lines noted in the description. Any insights wold be welcome. |
To clarify, I couldn't figure out how to get docker compose up to work with vite. I had to initialize with docker compose, then restart containers(without docker compose )and run the webserver (nodemon) and dev server (vite) manually. |
@ChrisMart21 Thanks for the ideas on how to run. Could you clarify the precise steps? Here is what I think the first two may be:
Is this correct? As for the third step, can you clarify exactly how you did the webserver and dev server? I want to figure out the right way to do it and be able to carefully document. Also, the VSC container will need to be modified to see if it can still work. |
Yeah those steps are what I did, to run the webserver I simply ran the npm script |
@hyperupcall I would very much like to merge this. Per comments above, can you let us know how you start OED? If I can reliably start with Vite and put in developer directions then I'm good with these changes. Many thanks and let me know if I can help in any way. |
@huss Thanks for the feedback, perhaps a little more work needs to be done before it's switched on as the default
Hmm, I only checked that the plotting feature works, so I might have missed that. It might be related to the proxy, that proxies requests from the frontend development server (on port
Right, both appear to work, but only one has the updated updated code. The one on Like you mention, you can go to port
Right yeah, it would probably be a good idea for it to print a special message when Vite is started in development. Something that tells the user to go to
Webpack can definitely be removed once this Vite support is merged. As you mention, probably would be good to keep both options around, until all documentation is changed and Vite support is working without issues for everyone
Right, I think reconsidering the install steps sounds like a good idea too. Along with taking less time to initialize the database, the "Install NPM dependencies" step in the same script could be improved. On my computer, it seems the heuristic for checking if "dependencies should be installed" is a bit strict (and often i have to wait a few seconds for dependencies "to install"). I always
Sounds good! I'll work on the following:
|
@ChrisMart21 @huss I'll check the scripts again to make sure things run in docker compose for everyone. For me, things work when running the dev server in I do remember experiencing issues and having to do extra things to make things work, but I don't remember any of them being specific to this PR. For example, the dockerfile for the OED server runs things as root, which means that the built files in |
For @hyperupcall First, thanks for your continued efforts on this important update that has been so complex. I see there are now merge conflicts. This is undoubtedly due to my clearing other PRs. Would you like me to resolve these and push an update or do you want to do it? I'm happy to help but want to give you the choice.
In case you didn't see if, I put in PR #1281 that you are welcome to comment on. It is an opt out for the DB initialize. The PR explains the rationalization. As for when OED does an NPM install, it is conservative for reasons similar to PR #1281 so inexperienced developers don't have issues. If you do your own npm install after a clone then OED should skip the npm build (but you probably want to do the ci option to avoid changing the OED setup). A lot of people don't know but the install switch --keep_node_modules will stop the npm install (see PR #1281 for directions but maybe it could be made easier to pass the commands). This is why the message says it skipped because unnecessary or as requested. I'm open to other ways to determine if the npm build is needed.
If this is tricky then I think it may be easy to print a message when running non-production. It could say that if you started with Vite then these are the ports. When we switch to Vite as the only option then it can be updated. What do you think? |
Description
This PR supercedes #879 and is related to #871. More information about the rational behind this change is contained in those two links.
This adds and configures Vite so OED can be build with it! To do so, look towards the following scripts in
package.json
:Replace
webpack
withvite
. Everything should work the same, and be much faster. The only difference is that withnpm run client:dev
, the development version of the site is NOT atlocalhost:3000
. It is atlocalhost:8085
. This is the reason why Vite is so fast - is that it bundles things that are only required and requested by the user. As a result, the user must access Vite's development server. (With Webpack, this option exist through a plugin, but is not required at all).There are three notable differences:
1.
index-webpack.html
The way Webpack and Vite are configured, they interact with
index.html
files in slightly different ways. So, until Webpack is removed, it seems easier to have two different "index.html" files. Vite readsindex.html
.2. Removed
OED_SUBDIR
This undocumented environment variable, seemed to be a workaround due to the previous location of the
index.html
. Previously,index.html
was located atsrc/client/index.html
(that's whyconfig.subdir
defaulted to/public
). Now,index.html
is located in the correct spot for both Webpack and Vite, andOED_SUBDIR
is no longer needed. This subdir value also adds a lot of complexity to the build step / static serving, so removing it makes things a lot more simple.3. Use of
string-replace-loader
As experienced in #879, both Webpack and Vite resolve imports differently. Moment is an example of this. When the import was changed to work with Vite, it stopped working with Webpack. I use
string-replace-loader
to do a simple string/regex substitution to Webpack works again.End
Because this change is opt-in, no documentation needs to be updated. If this change is merged, I was wondering what was the path forward is? I'm not sure how much documentation and other things need to be done, but the actual removal of Webpack should be relatively straight-foward.
Type of change
Checklist