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

build: [M3-5912] - Vite #8838

Merged
merged 47 commits into from
Mar 8, 2023
Merged

Conversation

bnussman-akamai
Copy link
Contributor

@bnussman-akamai bnussman-akamai commented Mar 2, 2023

Description πŸ“

  • Uses Vite for
    • Cloud Manager dev server and production builds
    • Storybook dev server and production builds
    • Cypress Tests (preprocessor)
  • Updates Storybook v7 (needed for Vite support)
  • Improves Storybook branding, theming, and dark mode support
  • Makes Cypress use a Vite preprocessor
    • Doing this so things like import.meta.env work

Todo β˜‘οΈ

  • Reintroduce New Relic
    • Maybe use the useScript hook or just put it in the index.html and let it run in dev?
  • Fix SSH key fingerprints
    • We'll probably need some kind of crypto library. I'm unsure if the native web crypto functions can do what we need
  • Fix algolia search
  • Get Cypress tests to run (works but may be unstable)
  • Get Storybook to run
    • Options include update to Storybook 7 and use vite builder or somehow define import.meta.env with current setup

Observations πŸ‘οΈ

  • Cypress e2e tests run stably
    • They seem to take a bit more time to run compared to 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)
    • When we start e2e testing with our internal CI, maybe we will benefit from doing a production build of CLoud Manager, serving it with serve and testing against that. This may be faster... cc @jdamore-linode
  • We should work to get our unit testing up to date. For now I am using swc to staticly define import.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 variables

How to test πŸ§ͺ

  • Checkout this PR
  • Before you run anything, I'd recommend running yarn from the repo's root just to get new dependencies installed and old ones removed
  • Run full suite of e2e tests with yarn cy:run
  • Generally test cloud manager's dev server using yarn up or yarn dev
  • Verify a Cloud Manager production build works
    • If an internal preview link is generated, it is safe to assume this works
  • Test storybook dev server with yarn storybook
  • Test storybook production build with yarn build-storybook (run from packages/manager)

@jdamore-linode
Copy link
Contributor

jdamore-linode commented Mar 2, 2023

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:

  • Running yarn up on this branch had Cloud Manager up and running in just shy of 30 seconds. Meanwhile, yarn up on develop took about 70 seconds (not including type checking). Really eager to see how fast it is on M1 devices.
  • I branched off your work and put together a really quick POC to get Cypress running. It's not perfect (I may have observed this issue once or twice, for example), but the few tests that I ran seemed to generally pass with these changes.

@bnussman
Copy link
Contributor

bnussman commented Mar 6, 2023

@cpathipa
Copy link
Contributor

cpathipa commented Mar 6, 2023

Nice work! @bnussman Great addition to DX and improves productivity.

Here are my observations:

  • Hot-reload is not working when changes made to serverHandler.ts (May be expected)

  • Below three specs failed upon running E2E suite.

image

@bnussman-akamai
Copy link
Contributor Author

@cpathipa

I found that reloading of serverHandler.ts works as I'd expect. The difference compared to webpack is that the page does not refresh anymore so data in the Redux / React Query store is the old data. Vite's reloading is working as expected, but good catch. This is a new behavior.

Screenshot 2023-03-06 at 5 28 49 PM

As for the e2e failures, I ran the tests that failed and they passed for me. I am hoping they were just flakey tests

@cpathipa cpathipa added Approved Multiple approvals and ready to merge! Needs More Approvals labels Mar 7, 2023
];

async function handleRequest(endpoint, filename) {
const response = await fetch(API_ROOT + endpoint + '?page_size=500');
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a 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 πŸš€

Copy link
Contributor

@mjac0bs mjac0bs left a 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 on develop 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.

@bnussman-akamai bnussman-akamai merged commit b193687 into linode:develop Mar 8, 2023
bnussman-akamai added a commit that referenced this pull request Mar 13, 2023
fix: Fixes the Radio styles caused by Vite #8838

Co-authored-by: Banks Nussman <banks@nussman.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants