Skip to content
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

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

hyperupcall
Copy link
Contributor

@hyperupcall hyperupcall commented Jun 11, 2024

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:

"client:dev": "npm run webpack:dev",
"client:build": "npm run webpack:build",
"client:build-dev": "npm run webpack:build-dev",

Replace webpack with vite. Everything should work the same, and be much faster. The only difference is that with npm run client:dev, the development version of the site is NOT at localhost:3000. It is at localhost: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 reads index.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 at src/client/index.html (that's why config.subdir defaulted to /public). Now, index.html is located in the correct spot for both Webpack and Vite, and OED_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

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

@hyperupcall hyperupcall mentioned this pull request Jun 11, 2024
5 tasks
@ChrisMart21
Copy link
Contributor

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!

@huss
Copy link
Member

huss commented Jun 11, 2024

A few points on this where I have thoughts/questions. I may have the ports/usage wrong so am confused.

  • When on 8085, not on the OED home page and after I save a change to the code the web browser page updates but does not work right. When this happens you are logged out of OED as an admin but not being logged in gives the same issue as does being on a page that should not be impacted by the change. Trying to login as admin on the page fails. A refresh of the web page does not help. I have to go to the home page (that gives a dashboard error screen) to refresh to see the changes correctly. This does not happen if I'm on the home page of OED and then update happens without issue. It seems to happen on all other pages. Do others see this? If not, I can provide more info.
  • When I run OED with Vite, it seems both port 3000 and 8085 work. I see src/vite.config.mjs has port 8085 and proxy 3000 but am not sure what that does. The PR talks about the dev version using 8085. Could you help me understand what is okay/should be used?
  • src/bin/www prints "Listening on port 3000" but the Vite changes this to 8085. Is there an easy way to fix if that makes sense?

These are notes that are for the record/future:

  • As this works well, a path to make Vite the default should be considered/planned. Assuming issues are not found in usage by others, OED should consider this change in the near future. Is there a reason to keep Webpack or can we get rid of that dependency?
  • With Vite being so fast, OED may want to reconsider some of the install steps. They have the advantage that they put the DB in a good state and recognize changes but they seem to slow down the install. Maybe a switch to skip the steps when desired so they happen by default to avoid unintended issues could be added. On one test it seems to take ~15 sec for these steps out of a total of ~23 to start OED. Not sure if this is a good idea or not so open to thoughts. The bigger savings of the rapid update during code changes is not impacted by this.
  • I'm not sure but subdir may have had something to do with a proxy or https early on. Just for the record and I'm fine with removing it.

Copy link
Member

@huss huss left a 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.

@huss
Copy link
Member

huss commented Jun 12, 2024

I tried running this some more and note this:

  • I'm starting OED with docker compose up.
  • I tried on Chrome and Firefox. They act similarly.
  • There is no need to make a change in the code to see issues. Refreshing the web page is enough.
  • I see this if not on the home/graphing page and I refresh the page:
    • If on localhost:3000 then it downloads index.html
    • If on localhost:8085 it give an incorrect page. It only clears if I go to the home/graphic page and refresh. As noted before, I get a dashboard error when I first go to the home/graphic page before the reload.

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.

@ChrisMart21
Copy link
Contributor

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.

@huss
Copy link
Member

huss commented Jun 12, 2024

@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:

  1. In the main OED directory run docker compose up to initialize the database and do an npm install. Then stop (^c) to shut down containers but leave them created.
  2. Use Docker (or VSC Docker extension) to start the oed-database and oed-web containers.

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.

@ChrisMart21
Copy link
Contributor

Yeah those steps are what I did, to run the webserver I simply ran the npm script
start:dev and vite:dev via vs code.
Attaching the 'web' container's terminal and running these commands with npm would work as well

@huss
Copy link
Member

huss commented Jun 19, 2024

@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.

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Jun 21, 2024

@huss Thanks for the feedback, perhaps a little more work needs to be done before it's switched on as the default

When on 8085, not on the OED home page and after I save a change to the code the web browser page updates but does not work right. When this happens you are logged out of OED as an admin but not being logged in gives the same issue as does being on a page that should not be impacted by the change. Trying to login as admin on the page fails. A refresh of the web page does not help. I have to go to the home page (that gives a dashboard error screen) to refresh to see the changes correctly. This does not happen if I'm on the home page of OED and then update happens without issue. It seems to happen on all other pages. Do others see this? If not, I can provide more info.

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 8085) to the actual OED server (on port 3000). I'll check that part out

When I run OED with Vite, it seems both port 3000 and 8085 work. I see src/vite.config.mjs has port 8085 and proxy 3000 but am not sure what that does. The PR talks about the dev version using 8085. Could you help me understand what is okay/should be used?

Right, both appear to work, but only one has the updated updated code. The one on 8085 is only for development, and is managed by Vite, and has the up-to-date bundled client code. This development server enables Vite to do things like hot-reloading. If we only configure server.port (and not server.proxy), then that would break things. Because the client code makes requests to /api/auth/various-routes, and the client code is being served by the Vite dev server on localhost:8085, the requests will simply 404. That's why server.proxy is configured that way, it tells Vite to identify all requests that start with /api, and proxy/re-route them to localhost:3000.

Like you mention, you can go to port 8085 and see the server, but that currently only serves the files that were created when building files (that are then saved to disk). Right now, all tasks except for npm run client:dev do that, bundle files to disk. So, if you remove all build files under src/client/public/app, you will find that port 3000 will no longer work (as planned).

src/bin/www prints "Listening on port 3000" but the Vite changes this to 8085. Is there an easy way to fix if that makes sense?

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 8085 to see their changes instead. In this case, we might as well serve a special index.html that says the same thing, to eliminate any confusion. But, it might be difficult to check on the OED server that the user started the Vite dev server (instead of using Webpack to build dev builds), so instead it could be simpler to just print more details and instruction, that covers the "starting vite in dev mode" use case.

As this works well, a path to make Vite the default should be considered/planned. Assuming issues are not found in usage by others, OED should consider this change in the near future. Is there a reason to keep Webpack or can we get rid of that dependency?

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

With Vite being so fast, OED may want to reconsider some of the install steps. They have the advantage that they put the DB in a good state and recognize changes but they seem to slow down the install. Maybe a switch to skip the steps when desired so they happen by default to avoid unintended issues could be added. On one test it seems to take ~15 sec for these steps out of a total of ~23 to start OED. Not sure if this is a good idea or not so open to thoughts. The bigger savings of the rapid update during code changes is not impacted by this.

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 npm install after cloning a new repo, or adding a new dependency or change things. But, I'm not sure if changing this part of the script will cause more "mysterious" errors for new contributors, or will require adding more documentation (that people may not read).

I'm not sure but subdir may have had something to do with a proxy or https early on. Just for the record and I'm fine with removing it.

Sounds good!

I'll work on the following:

  • Look into weird auth / admin logging on errors
  • In ./src/bin/www, better communicate which port should be visisted to see changes

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Jun 21, 2024

@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 docker (docker compose) and on the host machine (without docker compose).

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 src/client/public/app and sometimes node_modules are owned by root (i had to sudo rm -rf things often when testing things out). But I'll checkout this branch from a clean development branch to check for errors or whatever

@huss
Copy link
Member

huss commented Jun 21, 2024

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.

With Vite being so fast, OED may want to reconsider some of the install steps. They have the advantage that they put the DB in a good state and recognize changes but they seem to slow down the install. Maybe a switch to skip the steps when desired so they happen by default to avoid unintended issues could be added. On one test it seems to take ~15 sec for these steps out of a total of ~23 to start OED. Not sure if this is a good idea or not so open to thoughts. The bigger savings of the rapid update during code changes is not impacted by this.

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 npm install after cloning a new repo, or adding a new dependency or change things. But, I'm not sure if changing this part of the script will cause more "mysterious" errors for new contributors, or will require adding more documentation (that people may not read).

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.

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 8085 to see their changes instead. In this case, we might as well serve a special index.html that says the same thing, to eliminate any confusion. But, it might be difficult to check on the OED server that the user started the Vite dev server (instead of using Webpack to build dev builds), so instead it could be simpler to just print more details and instruction, that covers the "starting vite in dev mode" use case.

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?

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.

3 participants