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

[DT-295] Migrate from Create React App to Vite #2779

Merged
merged 9 commits into from
Feb 13, 2025

Conversation

fboulnois
Copy link
Contributor

@fboulnois fboulnois commented Feb 12, 2025

Addresses

https://broadworkbench.atlassian.net/browse/DT-295

Summary

Migrates from Create React App to Vite. Requires #2780 to go in first.


Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@fboulnois fboulnois requested a review from a team as a code owner February 12, 2025 15:47
@fboulnois fboulnois requested review from rushtong and s-rubenstein and removed request for a team February 12, 2025 15:47
@fboulnois fboulnois force-pushed the fb-dt-295-migrate-to-vite branch from d07534e to ff01968 Compare February 12, 2025 15:49
// https://vitejs.dev/config/
export default defineConfig({
plugins: [
react({ include: /\.(mdx|js|jsx|ts|tsx)$/ }),
Copy link
Contributor Author

@fboulnois fboulnois Feb 12, 2025

Choose a reason for hiding this comment

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

The include is required to also support mdx files

],
assetsInclude: ['**/*.md'],
build: {
outDir: 'build',
Copy link
Contributor Author

@fboulnois fboulnois Feb 12, 2025

Choose a reason for hiding this comment

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

Required to generate builds in the same location

assetsInclude: ['**/*.md'],
build: {
outDir: 'build',
target: 'es2022'
Copy link
Contributor Author

@fboulnois fboulnois Feb 12, 2025

Choose a reason for hiding this comment

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

Required to avoid an error with top-level awaits

target: 'es2022'
},
server: {
port: 3000,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required to keep compatibility with CRA

Copy link
Contributor

Choose a reason for hiding this comment

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

Can I suggest that we add a host and https block to server?

    host: 'local.dsde-dev.broadinstitute.org',
    https: {
      key: fs.readFileSync(`./server.key`),
      cert: fs.readFileSync(`./server.crt`),
    },

This also needs a new import

import fs from 'fs';

This starts up similarly to how we're running the app now:

  VITE v6.1.0  ready in 106 ms

  ➜  Network: https://local.dsde-dev.broadinstitute.org:3000/
  ➜  press h + enter to show help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like key and cert can directly take file paths, so the fs import is not required.

src/libs/tosService.jsx Outdated Show resolved Hide resolved
import {BrowserRouter} from 'react-router-dom';

const load = async () => {
unregister();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This functionality is deprecated.

@@ -91,5 +92,6 @@
],
"engines": {
"node": ">=22.11.0"
}
},
"type": "module"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non-module builds of Vite are deprecated.

@@ -41,10 +41,9 @@
},
"scripts": {
"analyze": "npm run build; source-map-explorer build/static/js/main.*",
"start": "react-scripts start",
"genschemas": "./scripts/compile-jsonschema.sh",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The genschemas script no longer exists.

<noscript>
You need to enable JavaScript to run this app.
</noscript>
<script type="module" src="./src/index.tsx"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only functional change to this file is adding this line.

@fboulnois fboulnois force-pushed the fb-dt-295-migrate-to-vite branch 2 times, most recently from 1f405d4 to 040664f Compare February 12, 2025 16:12
@@ -15,7 +15,9 @@ COPY types /usr/src/app/types
COPY public /usr/src/app/public
COPY package.json /usr/src/app/package.json
COPY package-lock.json /usr/src/app/package-lock.json
COPY index.html /usr/src/app/index.html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file moved location, and therefore needs to be included in the build.

COPY tsconfig.json /usr/src/app/tsconfig.json
COPY vite.config.js /usr/src/app/vite.config.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new file that needs to be included in the build.

@fboulnois fboulnois force-pushed the fb-dt-295-migrate-to-vite branch from 040664f to 32a7b69 Compare February 12, 2025 16:18
Comment on lines +44 to +46
"start": "vite",
"build": "vite build",
"preview": "vite preview",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the new vite commands.

@@ -81,6 +81,7 @@
"source-map-explorer": "2.5.3",
"typescript": "4.9.5",
"typescript-eslint": "7.17.0",
"vite": "6.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the latest versions of the react plugin and vite.

@fboulnois fboulnois marked this pull request as draft February 12, 2025 19:39
@fboulnois fboulnois force-pushed the fb-dt-295-migrate-to-vite branch from 9a1414c to 3e888d3 Compare February 12, 2025 21:00
@@ -13,8 +13,8 @@ module.exports = defineConfig({
component: {
specPattern: ['**/*.spec.js', '**/*.spec.jsx', '**/*.spec.ts', '**/*.spec.tsx'],
devServer: {
framework: 'create-react-app',
bundler: 'webpack',
framework: 'react',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new Vite config for Cypress.

@@ -2,7 +2,7 @@
<browserconfig>
<msapplication>
<tile>
<square150x150logo src="%PUBLIC_URL%/images/favicon/mstile-150x150.png"/>
<square150x150logo src="/images/favicon/mstile-150x150.png"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

%PUBLIC_URL% is a CRA-specific path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This functionality is deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are actually JSX files.

@fboulnois fboulnois marked this pull request as ready for review February 12, 2025 21:06
Copy link
Contributor

@rushtong rushtong 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 👍🏽

@otchet-broad otchet-broad self-requested a review February 12, 2025 23:10
Copy link
Contributor

@otchet-broad otchet-broad left a comment

Choose a reason for hiding this comment

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

Nice job!

@fboulnois fboulnois merged commit 8b20932 into develop Feb 13, 2025
9 checks passed
@fboulnois fboulnois deleted the fb-dt-295-migrate-to-vite branch February 13, 2025 16:05
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.

4 participants