-
Notifications
You must be signed in to change notification settings - Fork 364
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
build: [M3-5912] - Vite #8838
build: [M3-5912] - Vite #8838
Conversation
This is really, really exciting! Awesome work! I don't have a ton of time to spend on this today, but just had a couple quick notes/observations:
|
Here are the two PRs blocking us from the latest Vite version |
Nice work! @bnussman Great addition to DX and improves productivity. Here are my observations:
|
I found that reloading of As for the e2e failures, I ran the tests that failed and they passed for me. I am hoping they were just flakey tests |
]; | ||
|
||
async function handleRequest(endpoint, filename) { | ||
const response = await fetch(API_ROOT + endpoint + '?page_size=500'); |
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.
Any reason for replacing axios
with fetch
here?
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 try to get rid of axios wherever possible. It offers little benefit over fetch and fetch is native to the runtime.
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.
Ok apart from the storybook issues we already talked about - I think this is good - Nice job 🚀
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.
- Running
yarn up
on this branch had Cloud Manager up and running in just shy of 30 seconds. Meanwhile,yarn up
ondevelop
took about 70 seconds (not including type checking). Really eager to see how fast it is on M1 devices.
On my M1: yarn up
now takes about 7 seconds, which is very cool. On develop
, it takes me about 40 seconds if we exclude type checking.
E2E tests and output of yarn commands looked good to me.
For the Storybook changes: New skin and dark mode toggle are looking nice. 🎉 I do think it feels a little cluttered/confusing for the side nav to contain links to both the Docs and the Canvas views of a story, especially when we include the ArgsTable
at the bottom of so many Docs pages, but that's not a reason to hold up merging this PR.
fix: Fixes the Radio styles caused by Vite #8838 Co-authored-by: Banks Nussman <banks@nussman.us>
Description 📝
import.meta.env
workTodo ☑️
useScript
hook or just put it in the index.html and let it run in dev?import.meta.env
with current setupObservations 👁️
develop
. I am pretty sure this is due to the fact that with vite development mode, loading the page can actually take a bit more time due to all of the modules the browser must load (vite works like this by design)serve
and testing against that. This may be faster... cc @jdamore-linodeimport.meta.env
just to the tests can run. This is fine for the most part because unit tests really shouldn't need access to any environment variablesHow to test 🧪
yarn
from the repo's root just to get new dependencies installed and old ones removedyarn cy:run
yarn up
oryarn dev
yarn storybook
yarn build-storybook
(run frompackages/manager
)