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

DSEGOG-341 Migrate from CRA to Vite #421

Merged
merged 31 commits into from
Sep 3, 2024

Conversation

joelvdavies
Copy link
Contributor

@joelvdavies joelvdavies commented Jul 26, 2024

Description

Migrates from CRA to Vite. Following on from ral-facilities/inventory-management-system#348 and ral-facilities/scigateway#1380.

Notes

  • Copied new tsconfig from IMS (The config here was copied to IMS in the first place)
  • Instances of process.env.REACT_ replaced with import.meta.env.VITE_
    • Renamed VITE_APP_OPERATIONSGATEWAY_BUILD_DIRECTORY to VITE_APP_OPERATIONS_GATEWAY_BUILD_DIRECTORY
    • Replaced REACT_APP_E2E_TESTING with VITE_APP_INCLUDE_MSW and VITE_APP_BUILD_STANDALONE (Same as IMS so could use the same config and allows more explicit differentiation of e2e tests with and without the api)
  • Some handlers in handlers.ts don't match the backend e.g. /sessions/:id doesn't have a duplicate check in the backend when it probably should (also on a related note the post's should really use 201's but the backend currently uses 200s.)
  • Like for IMS I have renamed setupTests.tsx to testUtils.tsx and moved MSW setup to a separate new setupTests.ts imported in the vitest config - to avoid repeated execution during imports)
  • Replaced jest-canvas-mock with vitest-canvas-mock
  • MSW v2 stores the url as a string in responses, so many tests that directly accessed that have been modified to first convert to a URL object before accessing the search params
  • Removed applyDatePickerWorkaround and cleanupDatePickerWorkaround from unit tests - I added this in datagateway for MUI v5 when we enzyme and it was copied over here, but doesn't seem like its needed anymore (and broke here anyway)
  • Like IMS, removed cypress-failed-log as its 2 years old and I don't believe it does anything anymore or otherwise the import is broken as it cannot use require
  • Moved msw to be a full dependency instead of a dev one - This is like IMS as the yarn tsc in the build process uses the tsconfig.build.json. While the mock handlers could be excluded from this, it will then also break any e2e tests relying on this build - the fix would be to have another tsconfig for this case, and adding another build command that is only used in this case. Given it should already be removed from the bundle at the end, I thought this was simpler.
  • yarn lint is not the same as on IMS - we include tests in the checks with tsc to resolve further typescript issues, but there are many here that have been ignored and I thought it best to leave to a further eslint v9 PR if addressed as a result
  • Was originally going to upgrade to eslint v9 at the same time as I thought the changes to eslint were more complicated, but was simple in practice so leaving to another PR to avoid making more changes here

Testing instructions

Add a set up instructions describing how the reviewer should test the code

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

Closes DSEGOG-341

@joelvdavies joelvdavies added enhancement New feature or request dependencies labels Jul 26, 2024
@joelvdavies joelvdavies self-assigned this Jul 26, 2024
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from 7985007 to acb5a81 Compare August 12, 2024 12:36
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from 9e25f4e to 34d3ad2 Compare August 13, 2024 10:39
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from 8dbfdc1 to 86fb22e Compare August 13, 2024 11:43
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from f295281 to f5e6450 Compare August 13, 2024 14:35
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from d42fccc to a01142c Compare August 13, 2024 15:39
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from 7c29057 to fd74309 Compare August 14, 2024 11:00
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from c29e391 to d1f672a Compare August 15, 2024 13:00
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from 0ec9fc2 to bcd7452 Compare August 15, 2024 14:23
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 95.01916% with 13 lines in your changes missing coverage. Please review.

Project coverage is 97.34%. Comparing base (7413fd8) to head (d20ee8f).
Report is 32 commits behind head on develop.

Files with missing lines Patch % Lines
src/testUtils.tsx 81.25% 9 Missing ⚠️
globalSetup.js 0.00% 2 Missing and 1 partial ⚠️
playwright.config.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #421      +/-   ##
===========================================
+ Coverage    95.35%   97.34%   +1.99%     
===========================================
  Files           83       86       +3     
  Lines         3830    10583    +6753     
  Branches      1092     1649     +557     
===========================================
+ Hits          3652    10302    +6650     
- Misses         172      277     +105     
+ Partials         6        4       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from 13cf5e9 to f30b107 Compare August 16, 2024 10:46
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from f30b107 to 683e819 Compare August 16, 2024 11:31
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from b83aefc to dc92290 Compare August 16, 2024 13:16
@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from 1d32886 to d960d45 Compare August 16, 2024 15:02
Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

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

The code is looking good aside from some minor comments of mine. Dev mode works great, as does tests. However, when running yarn preview:dev:build - I can't load OG in the SG react 18 branch. I'm getting an Uncaught SyntaxError: import declarations may only appear at top level of a module error, and it's pointing to a React import. Might be me doing something wrong but thought I'd raise it!

package.json Outdated Show resolved Hide resolved
src/images/imageWindow.component.test.tsx Outdated Show resolved Hide resolved
src/main.tsx Outdated Show resolved Hide resolved
src/plotting/plot.component.tsx Outdated Show resolved Hide resolved
src/plotting/plotSettings/yAxisTab.component.tsx Outdated Show resolved Hide resolved
src/testUtils.tsx Outdated Show resolved Hide resolved
src/testUtils.tsx Outdated Show resolved Hide resolved
src/traces/tracePlot.component.tsx Outdated Show resolved Hide resolved
vite.config.ts Show resolved Hide resolved
@joelvdavies
Copy link
Contributor Author

joelvdavies commented Sep 2, 2024

The code is looking good aside from some minor comments of mine. Dev mode works great, as does tests. However, when running yarn preview:dev:build - I can't load OG in the SG react 18 branch. I'm getting an Uncaught SyntaxError: import declarations may only appear at top level of a module error, and it's pointing to a React import. Might be me doing something wrong but thought I'd raise it!

Will address the other comments in the morning. Just spent a while debugging this with Joshua and finally got to the cause. Its the same issue you noticed on IMS, only there it was intermittent - almost every other build it did came out with imports in the main.js that shouldn't have been there breaking the build, only it seems that in the past few months an update has caused it to become a permanent issue not an intermittent one. I now believe as I was not specifying a format in the build, it used a default of es and umd. But due to the output filename these were both output to the same file. By forcing it to one of them at a time I have found that yarn build does this too if I use es instead of umd, so I think the IMS issue was caused by them producing both files and depending on speed each would be used only some of the time. Now yarn build seems to cause it to use umd and build --watch uses es. I am not clear on why the es modules build is doing this when it builds as a library - as far as I am aware it should be capable of it as I only used the example from the vite documentation. But for now the obvious solution is to use umd for the build for scigateway. So es modules are only used through dev (where the speed is beneficial) and in scigateway where this doesn't cause a problem. Hopefully a future update will fix whatever this is (although it could be my own misunderstanding of the config as well).

@louise-davies
Copy link
Member

@joelvdavies I believe the issue with the es build is we're not expecting an es module when loading the plugin from SciGateway - you need special handling to be able to handle es modules (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#applying_the_module_to_your_html)

If we really wanted es modules, it's probably worth waiting until everything is migrated over to Vite, and then overhauling the SG plugin loading system to be able to handle es modules (either in replacement of handling non-modules, or be able to load either using a feature switch in the plugin config?)

Either way, switching to UMD solves the immediate problem 👍

@joelvdavies joelvdavies force-pushed the DSEGOG-341-migrate-cra-to-vite branch from 288e650 to d20ee8f Compare September 3, 2024 09:24
Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

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

With the changes to the build, I'm happy that everything works (i.e. dev, loading production build in SG, e2e tests, unit tests) - at least for me.

Copy link
Contributor

@joshuadkitenge joshuadkitenge left a comment

Choose a reason for hiding this comment

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

LGTM tested dev, loading production build in SG, e2e tests, unit tests and e2e tests

@joelvdavies joelvdavies merged commit 1ce6e85 into develop Sep 3, 2024
6 checks passed
@joelvdavies joelvdavies deleted the DSEGOG-341-migrate-cra-to-vite branch September 3, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants