Skip to content

Conversation

joelvdavies
Copy link
Contributor

@joelvdavies joelvdavies commented Mar 12, 2024

Description

Replaces CRA with Vite. Used the default settings that create-vite would have generated in the tsconfig. The only real change caused by that is a requirement of underscores on some unused variable & I removed the props on the footer as they weren't either (noUnusedParameters).

Notes

Warning

The index.html has moved out of the public folder into the root, so docker-ims needs to be updated on gitlab as the title can no longer be replaced as it used to be, and instead will be in /dist after being built. Likely also effects any ansible config.

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
  • Test against other front ends

Agile board tracking

Closes #1379

@joelvdavies joelvdavies added enhancement New feature or request dependencies Pull requests that update a dependency file labels Mar 12, 2024
@joelvdavies joelvdavies self-assigned this Mar 14, 2024
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 94.82759% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 86.42%. Comparing base (b51eecf) to head (bf9dce6).

Files Patch % Lines
src/i18n.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           react-18-#1205    #1380       +/-   ##
===================================================
- Coverage           96.76%   86.42%   -10.35%     
===================================================
  Files                  48       56        +8     
  Lines                1763     8561     +6798     
  Branches              497      893      +396     
===================================================
+ Hits                 1706     7399     +5693     
- Misses                 53     1155     +1102     
- Partials                4        7        +3     

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

@joelvdavies joelvdavies force-pushed the migrate-to-vite-#1379 branch from 9882f3b to 47c20d0 Compare March 15, 2024 11:29
@joelvdavies joelvdavies force-pushed the migrate-to-vite-#1379 branch from 47c20d0 to 7cebf93 Compare March 15, 2024 11:37
@joelvdavies joelvdavies force-pushed the migrate-to-vite-#1379 branch from 7747b8b to 505728c Compare March 21, 2024 10:32
@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.59%. Comparing base (7477db6) to head (75ccf21).
Report is 39 commits behind head on develop.

Files with missing lines Patch % Lines
src/i18n.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1380      +/-   ##
===========================================
+ Coverage    97.13%   97.59%   +0.46%     
===========================================
  Files           48       53       +5     
  Lines         1814     5614    +3800     
  Branches       496      608     +112     
===========================================
+ Hits          1762     5479    +3717     
- Misses          48      134      +86     
+ Partials         4        1       -3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

louise-davies
louise-davies previously approved these changes Dec 3, 2024
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.

LGTM and it's working well with OG. However, I don't want to merge this yet before we're ready to make the necessary deployment changes (e.g. scigateway-ansible).

I can probably make a branch for the changes for scigateway-ansible to support this work and merge it in once we are ready to switch to v3.x.x in production.

I've created an issue in scigateway-ansible: ral-facilities/scigateway-ansible#10

joshdimanteto
joshdimanteto previously approved these changes Dec 3, 2024
Copy link
Contributor

@joshdimanteto joshdimanteto 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 with IMS

@joelvdavies joelvdavies dismissed stale reviews from joshdimanteto and louise-davies via 22daf73 March 17, 2025 09:02
@joelvdavies
Copy link
Contributor Author

joelvdavies commented Mar 17, 2025

Note: Vite v6 support added to react router v7 (So still using v5 here)

joshdimanteto
joshdimanteto previously approved these changes May 9, 2025
Copy link
Contributor

@joshdimanteto joshdimanteto 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 with IMS

@joelvdavies joelvdavies requested a review from louise-davies May 20, 2025 14:00
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.

Just realised there's a lingering reference to build instead of dist - in .github/workflows/release-build.yml, on this step:

 - name: Create tarball
        run: |
          mv build scigateway-$TAG_NAME
          tar -czf scigateway-$TAG_NAME.tar.gz scigateway-$TAG_NAME

We need to switch the mv build to mv dist as otherwise the built code won't get bundled for the release tar

@joelvdavies joelvdavies requested a review from louise-davies May 21, 2025 09:05
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.

LGTM - I have deployed this on datagateway-dseg and it works well :)

Copy link
Contributor

@joshdimanteto joshdimanteto left a comment

Choose a reason for hiding this comment

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

Tested with operations gateway and IMS. LGTM

@louise-davies
Copy link
Member

@joelvdavies I think this is ready to go in now that I've tested deploying it and the corresponding DataGateway PR got merged

@joelvdavies joelvdavies merged commit 3853bcf into develop Jun 3, 2025
6 checks passed
@joelvdavies joelvdavies deleted the migrate-to-vite-#1379 branch June 3, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate CRA to Vite

4 participants