-
Notifications
You must be signed in to change notification settings - Fork 56
Build with Vite #1396
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
base: dev
Are you sure you want to change the base?
Build with Vite #1396
Conversation
@@ -16,9 +14,5 @@ | |||
</head> | |||
<body> | |||
<div id="main"></div> | |||
<script> |
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 will inject hot reloading code similar to this content in development mode (yarn start
), so this is no longer needed.
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 for the explanation!
// index.html is placed at the root of the repo for Vite to pick up. | ||
customFile('HTML_FILE', './index.html', './lib/index.tpl.html') | ||
customFile('CUSTOM_CSS', './tmp/custom-styles.css', './example/example.css') | ||
customFile('YAML_CONFIG', './tmp/config.yml', './example/example-config.yml') |
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.
Miles says hot reloading from the original customizations files is important.
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 @daniel-heppner-ibigroup's suggestion of doing a symlink instead of a copy is a good idea!
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.
@miles-grant-ibigroup and @daniel-heppner-ibigroup The old craco.config.js does copy the JS configs (and custom graphql) files. We can either: keep copying those files, so that no changes in the imports in those files are needed, or go ahead with symlinks, but that requires updating the imports of config.js that use OTP-RR relative paths. What do you think?
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.
@miles-grant-ibigroup and @daniel-heppner-ibigroup To avoid modifying custom files, I added a watch so it copies edited custom files over for hot reloading (e2d2774).
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.
Nice fix. Seems just as good as a symlink
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.
It's so fast!
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 working really well. I am glad we resolved the config hot reloading issue so easily. I'm certainly impressed!
@@ -18,6 +17,7 @@ import { | |||
ItineraryView | |||
} from '../util/ui' | |||
import { getModesForActiveAgencyFilter, getUiUrlParams } from '../util/state' | |||
import { loadLocaleData } from '../util/i18n-loader' |
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.
Do you remember what the purpose of moving this into its own file was? Just curious
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.
Yes, it is to isolate the parts that use the Vite-specific glob imports. This file is mocked for tests (tests continue to use Babel).
@@ -16,9 +14,5 @@ | |||
</head> | |||
<body> | |||
<div id="main"></div> | |||
<script> |
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 for the explanation!
// index.html is placed at the root of the repo for Vite to pick up. | ||
customFile('HTML_FILE', './index.html', './lib/index.tpl.html') | ||
customFile('CUSTOM_CSS', './tmp/custom-styles.css', './example/example.css') | ||
customFile('YAML_CONFIG', './tmp/config.yml', './example/example-config.yml') |
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.
It's so fast!
Description
This PR sets Vite as the new build system.
We place customization files (CSS, JS, GraphQL) in the /tmp folder so that imports can use relative paths within the repo root. index.html is copied to the repo root for Vite to pick up, and gets some pre-processing (inject the main script and remove HTML comments).
Other things that we used to configure explicitly that are now included out-of-the-box:
yarn start
works out of the box, even with no environment variables passed (it will use the example subfolder).Things that were dropped:
require
syntaxThings that don't change:
mastarm deploy
seems to still be working (all content in dist is flat, no subfolders)PR Checklist: