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

frontend to vite #3860

Merged
merged 72 commits into from
Sep 9, 2024
Merged

frontend to vite #3860

merged 72 commits into from
Sep 9, 2024

Conversation

jmetev1
Copy link
Collaborator

@jmetev1 jmetev1 commented Aug 17, 2024

Things I did

  1. changed to vite and vitest
  2. removed some unneeded stuff from integration workflow
  3. moved to node alpine bc couldn’t make it work with corepack and new type= module nature of vite
  4. renamed env variables for vite
  5. add global browser fetch in tests which was happening before behind the scenes with CRA
  6. not running "src/features/auth/slices/authenticationSlice.test.ts" bc ES Modules cannot be stubbed so need to find other solution for sinon.

Outcomes

  1. can’t use require anymore unless you name file .cjs or .cts. Everything is default import export now.
    closes Migrate frontend build to vite #3785

Test Links:

Landing Page
MoreCast
Percentile Calculator
C-Haines
FireBat
FireBat bookmark
Auto Spatial Advisory (ASA)
HFI Calculator

@jmetev1 jmetev1 marked this pull request as draft August 17, 2024 22:40
@jmetev1 jmetev1 changed the title new repo clone frontend to vite Aug 20, 2024
@conbrad
Copy link
Collaborator

conbrad commented Sep 5, 2024

My numbers are improving now that one less test is disabled! image

Nice! How many tests are disabled? The code coverage reports make sense when I'm looking at the ones with coverage or partial coverage, but a bunch of missing coverage around HFI Calculator signals that either those cypress tests are disabled or the coverage report isn't being merged properly.

@jmetev1
Copy link
Collaborator Author

jmetev1 commented Sep 5, 2024

My numbers are improving now that one less test is disabled! image

Nice! How many tests are disabled? The code coverage reports make sense when I'm looking at the ones with coverage or partial coverage, but a bunch of missing coverage around HFI Calculator signals that either those cypress tests are disabled or the coverage report isn't being merged properly.

image whoops!

@jmetev1 jmetev1 marked this pull request as ready for review September 5, 2024 18:20
@conbrad conbrad requested review from brettedw and dgboss September 5, 2024 18:29
Copy link
Collaborator

@conbrad conbrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, amazing work thanks for the considerable amount of effort that went into this! Builds are faster and we can sever our dependency on cra.

On my side just the couple comments I added this morning to be resolved, and the injected error to test sentry source maps and I'm happy. I'll let @dgboss and @brettedw carve out some time to review further.

@@ -14,16 +14,6 @@ const store = configureStore({
getDefaultMiddleware({ immutableCheck: false, serializableCheck: false }).concat(thunkMiddleware)
})

if (process.env.NODE_ENV === 'development' && module.hot) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if we get the equivalent behaviour with vite out of the box? Or will we have to re-start our debugging session if we are making changes to a reducer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm. I feel like vite should handle this well since a big focus of it is not having to rebundle stuff https://vitejs.dev/guide/why.html#why-vite . But i haven't done much reducer stuff lately. Can you break down for me what problem this was solving? you don't want to lose some state in redux?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious more than anything so not a big deal. I think you are correct in that it prevented loss of state in redux. I don't think we need to do anything or look into this any further. Thanks!

Copy link
Collaborator

@dgboss dgboss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! Thank you for all the time you put into this!

Copy link

sonarcloud bot commented Sep 5, 2024

Copy link
Collaborator

@brettedw brettedw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the effort you put into this @jmetev1, love it!

@conbrad conbrad temporarily deployed to production September 9, 2024 17:23 Inactive
@conbrad conbrad merged commit 2aa5bd3 into main Sep 9, 2024
28 checks passed
@conbrad conbrad deleted the task/vitejs branch September 9, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate frontend build to vite
4 participants