-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
1778 migrate from webpack to vite #1822
base: main
Are you sure you want to change the base?
Conversation
renders map!
should have been included in 0e9e465; I think I missed this because I was running scripts with pnpm
Review ETA: Fri 9/13 at 2PM PT |
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 running great! Appreciate your time in migrating from Webpack to Vite. There's a noticable difference in how quickly the app loads on changes.
Leaving a note on using Babel in the future.
@vitejs/plugin-react provides Babel plugins/presets.
Docs for @vitejs/plugin-react
Changes to running the application locally as of today.
Updates for developers:
(refer to #1824 to remove this step)
- Update .env file
- Pre-pend variable with
VITE_
, e.g.:VITE_MAPBOX_TOKEN=..
- Pre-pend variable with
Update README.md
- Change
Visit http://localhost:3000/
toVisit http://localhost:5173/
Commenting an approval for this branch. Once we've decided on a solution for #1824 and @ryanfchase reviews, it's good to merge. |
General Notes for PR Documentation
|
Thanks for the docs @bphan002! I understand that not commit messages alone aren't the best way to document changes lol.
|
Made some updates today to address #1824. Everyone who develops 311-Data will receive an alert from their browser when running locally if they haven't renamed their environment variables to be compatible with Vite (or if their .env file doesn't exist). This alert also includes instructions to automatically fix this by running the checkEnv.js script. Display an alert in local dev server if
|
const url = `https://graphql.contentful.com/content/v1/spaces/${import.meta.env.VITE_CONTENTFUL_SPACE}`; |
311-data/hooks/useContentful.js
Line 15 in 26599df
Authorization: `Bearer ${import.meta.env.VITE_CONTENTFUL_TOKEN}`, |
Modifed checkEnv.js to rename env vars for Vite if necessary
Screenshot below shows the terminal output of npm run check-env
, with the following initial conditions in my .env file:
MAPBOX_TOKEN
,CONTENTFUL_SPACE
using their old names (not prefixed withVITE_
)- deleted
HUGGINGFACE_LOGIN_TOKEN
, to validate that there weren't any regressions on the current behavior of checkEnv.js
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.
Tested this last week, this is looking good! Approved! 🎉
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.
see comment here: #1778 (comment)
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.
Approving after showing that specifying the base
URL fixes the missing assets problem:
- changing
base
to match my repo's name: migrate build tooling from webpack to Vite #1778 (comment) - migrate build tooling from webpack to Vite #1778 (comment)
closes #1778
closes #1824
main
branchAny questions? See the getting started guide