- 
                Notifications
    You must be signed in to change notification settings 
- Fork 70
Use Vite for build #1218
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
Use Vite for build #1218
Conversation
This required updating Vue I18n from v9 to v10. package-lock.json will be updated in the next commit.
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.
Notes for code review
| #!/bin/bash -eu | ||
| set -o pipefail | ||
|  | ||
| cp index.html public/ | 
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.
Vite wants index.html at the root of the repository, but Vue CLI wants it in public/. My solution is to copy index.html into public/ during testing.
        
          
                test/run.sh
              
                Outdated
          
        
      | cp index.html public/ | ||
| output=$(mktemp) | ||
| trap 'rm -- "$output"' EXIT | ||
| trap 'rm -f -- public/index.html "$output"' EXIT | 
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.
I'm specifying -f on the off-chance that public/index.html is removed by another process. Even if public/index.html no longer exists, I want to make sure that $output is removed. -f will ignore an error in removing public/index.html and proceed to remove $output.
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.
-fwill ignore an error in removing public/index.html and proceed to remove$output.
Hmm I guess rm does that by default. 😅 I'll go ahead and remove -f.
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.
I don't think the trap cares about the status code returned by rm. We just need it to remove the files.
| // Adding `logger` in part in order to silence certain logging during testing. | ||
| logger = console | ||
| logger = console, | ||
| buildMode = import.meta.env?.MODE ?? 'production' | 
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.
Right now, NODE_ENV is the only environment variable we really use. However, Vite has a different, related concept of "modes." Environment variables are also accessed differently from before: via import.meta.env rather than process.env. After reading about it, I thought it'd be ideal to use Vite modes rather than NODE_ENV.
Since testing is still using Vue CLI, it can't access environment variables from import.meta.env. We could introduce logic or a utility file to choose between import.meta.env or process.env, but I wanted to avoid that. It felt cleaner to put buildMode in the container. It's then easy for testing to specify a different value for it. In testing, import.meta.env is undefined (but import.meta isn't, which sort of surprised me).
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.
import.meta is defined by ES modules and vite adds custom property env in it.
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.
Aha, thank you. 👍
| }, | ||
| // VUE_APP_OIDC_ENABLED is not set in production. It can be set during local | ||
| // development to facilitate work on SSO. | ||
| oidcEnabled: process.env.VUE_APP_OIDC_ENABLED === 'true', | 
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.
I think it's the right time to stop using this environment variable. There are now two ways to run Frontend in development: npm run dev and npm run dev:build. I didn't want to multiply that by two for OIDC vs. not. I've been planning to go in this direction anyway: see #1038.
        
          
                Procfile
              
                Outdated
          
        
      | @@ -1,2 +1,3 @@ | |||
| vue: vue-cli-service build --mode development --watch | |||
| vite: vite dev | |||
| build: NODE_ENV=development vite build --mode development --watch | |||
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.
Even though we're mostly using Vite modes now, we still need to specify NODE_ENV=development, because vite.config.js references NODE_ENV. vite dev specifies NODE_ENV=development automatically, which is why only this line has it.
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.
we can use [conditional config](https://vite.dev/config/#conditional-config] and remove NODE_ENV from here
|  | ||
| server { | ||
| listen 8989; | ||
| listen 8686; | 
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.
8989 is now the Vite dev server. 8686 is nginx.
| "vue": "~3", | ||
| "vue-flatpickr-component": "~9", | ||
| "vue-i18n": "^9.2.2", | ||
| "vue-i18n": "^10.0.7", | 
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.
For some reason, I had trouble with Vue I18n until I updated the dependency. I'm not sure why that is. I definitely had i18n working the last time I was working on Vite things. In any case, it's probably a good thing to be on a more recent version of Vue I18n. This is the latest v10 version of Vue I18n. It's not easy to update to v11 quite yet: see getodk/central#1026.
| }, | ||
| "devDependencies": { | ||
| "@intlify/vue-i18n-loader": "~4", | ||
| "@intlify/unplugin-vue-i18n": "^6.0.8", | 
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.
#671 has a few small notes about Vue I18n. I'm not quite sure, but I think when I wrote the issue, I was imagining that unplugin-vue-i18n was a replacement for vue-i18n-loader. In actuality, I think it's a replacement for vue-cli-plugin-i18n, which itself uses vue-i18n-loader. I was able to remove all i18n config from the Vue CLI config file, vue.config.js. unplugin-vue-i18n can be used with either Vite (in vite.config.js) or webpack (karma.conf.js).
| </template> | ||
|  | ||
| <script setup> | ||
| import { defineOptions, defineAsyncComponent, watchEffect, computed } from 'vue'; | 
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.
The Vite build included a warning about importing defineOptions. The Vue build handles it specially, so it doesn't need to be imported.
| // EXPORT | ||
|  | ||
| export default () => createI18n({ | ||
| allowComposition: true, | 
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.
I think this is the default now? The release notes for Vue I18n v10 says that this option is no longer supported.
951ec47    to
    6ca24e1      
    Compare
  
    | Unfortunately, I'm getting test timeouts in CircleCI for some reason. Tests are running fine for me locally. Locally, I'm not sensing that the tests are slower than they were before. In CircleCI, it usually seems to be the tests of  | 
| I have been looking into test failure. I think this could be due to recent CircleCI changes https://discuss.circleci.com/t/docker-executor-infrastructure-upgrade/52282. In that document they are saying that they have changed cgroupv1 to cgroupv2 and there is a suggested fix for Jest: upgrade to node 23.1+ or specify the maximum number of workers. I have tried running the job with ssh to see what's happening and noticed that when we run  One idea to try is to run karma against build generated by vite and get rid of webpack altogether | 
- Run tests in batches on CircleCI. - Allow a low number of warnings.
| @@ -1,15 +1,46 @@ | |||
| #!/bin/bash -eu | |||
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.
I got rid of -u because it was complaining locally that $CIRCLECI wasn't set.
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.
we can use default value of $CIRCLECI if not set like this ${CIRCLECI-}
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.
Ah perfect, I thought there must be something like that. I'll make that change.
| # warnings; and warnings from Karma. | ||
| set -- -F -e 'WARN LOG:' -e 'ERROR LOG:' -e 'Module Warning' -e 'WARN [web-server]:' "$output" | ||
| warnings=$(grep -c "$@") | ||
| if (( $warnings > 4 )); then | 
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.
These are the 4:
Feature flag __VUE_PROD_HYDRATION_MISMATCH_DETAILS__ is not explicitly defined. You are running the esm-bundler build of Vue, which expects these compile-time feature flags to be globally injected via the bundler config in order to get better tree-shaking in the production bundle.
[intlify] 'tc' and '$tc' has been deprecated in v10. Use 't' or '$t' instead. 'tc' and '$tc’ are going to remove in v11.
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.
I am very excited for this change, specially the auto reloading feature.
General observations:
- Getting missing 'bootstrap.css.map` while running dev server locally. Not super important though.
- I am assuming that if we proxy to vite dev server from nginx then auto reloading would stop working. I wish nginx would have been the entry point of the application during development or remove nginx altogther and have vite do the proxy to backend and enketo.
- distdirectory size (excluding .map files) reduced from 5.7mb to 5.6mb.
- Because in production mode vite doesn't generate map files, build is faster and this will be helpful in building docker image as well.
        
          
                Procfile
              
                Outdated
          
        
      | @@ -1,2 +1,3 @@ | |||
| vue: vue-cli-service build --mode development --watch | |||
| vite: vite dev | |||
| build: NODE_ENV=development vite build --mode development --watch | |||
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.
we can use [conditional config](https://vite.dev/config/#conditional-config] and remove NODE_ENV from here
| vue(), | ||
| VueI18nPlugin({ | ||
| include: resolve(dirname(fileURLToPath(import.meta.url)), './src/locales/**'), | ||
| compositionOnly: false, | 
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.
Maybe in some point in future we can only use composition APIs and remove this line
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.
I feel like we've been slowly migrating our components from the Options API to the Composition API, which I think is a good thing. Even if we continue to use the Options API for some components, those components can still use setup() I think. We'll have to remove this option once we move to v12 of Vue I18n: see getodk/central#1027.
| 
 I'll file an issue about that. 👍 
 I actually didn't try to proxy to the Vite dev server from nginx. I'd be happy for us to do that, but like you, I assume we'd run into issues with HMR. Why would it be beneficial for nginx to be the entry point? I figured that as long as the port is consistent with how it was before (8989), it doesn't matter too much which server it's hitting. Though I guess it's awkward that  I thought about having Vite proxy to Backend and Enketo directly rather than through nginx, but I feel like there's a lot going on in the nginx config. For Enketo, there are the redirects to Frontend. For Backend, nginx removes the  | 
| @@ -1,15 +1,46 @@ | |||
| #!/bin/bash -eu | |||
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.
we can use default value of $CIRCLECI if not set like this ${CIRCLECI-}
| // Adding `logger` in part in order to silence certain logging during testing. | ||
| logger = console | ||
| logger = console, | ||
| buildMode = import.meta.env?.MODE ?? 'production' | 
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.
import.meta is defined by ES modules and vite adds custom property env in it.
| "dev:oidc": "VUE_APP_OIDC_ENABLED=true npm run dev", | ||
| "build": "vue-cli-service build", | ||
| "dev": "nf start vite,nginx", | ||
| "dev:build": "nf start build,nginx", | 
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.
what are the use cases of this?
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.
This is meant to work similarly to how it does today. Files rebuild automatically, but there's no HMR. Personally, sometimes I like to have a browser tab open that reflects one version of the code and another tab open that reflects another. I can compare the two. In that case, I like that the browser doesn't try to automatically refresh either tab as I make changes.
| 
 My temporary solution was to run tests in batches. Hopefully this won't be an issue once we move to Vitest. If it is, that might be more of a sign that there's a memory leak or something like that. I also wanted to note that CircleCI is now a bit slower than before. CircleCI ran in 1m 56s for the latest commit on  | 
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.
This looks good to me 🎉
Closes #671. With this PR:
Given that testing will continue to use Vue CLI for now, we need Frontend to work with both Vite and Vue CLI. That might sound hard, but it ended up working out OK. I'm happy to separate the changes here from the set of changes for moving to Vitest, which are at least as big as this PR.
What has been done to verify that this works as intended?
npm run dev:build. I see that the homepage is rendered.Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes