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

feat: replace CRA with Vite [LIBS-598] #847

Open
wants to merge 31 commits into
base: alpha
Choose a base branch
from

Conversation

KaiVandivier
Copy link
Contributor

@KaiVandivier KaiVandivier commented Apr 27, 2024

Part 1 of the changes -- LIBS-598

Includes:

  1. Starting and building an app with Vite
  2. Compiling and handling service workers (using an existing webpack script we have)
  3. react-scripts is removed
  4. A number of small fixes along the way to make those work
    1. Adjusted moment locales import to ES module form and handled that in the Jest config
    2. Change the file watching/compilation process for apps to allow .[jt]sx extensions in app files
    3. Updated the shell and adapter files to have .jsx extensions
    4. Added a fix to @dhis2/cli-helpers-engine to let Vite's dev server use the console, which enables helpful interactive output

Following changes will look like:

  1. Building and serving plugins (including PWA for them)
  2. Build libraries with Vite
  3. Making sure unit tests are working
  4. Making the Vite config extensible for consumers (I think this will be a big help for the Tracker team, who could add config support Flow types)
  5. Small miscellany: clean up and refactoring env var management, for example

If I remember right, these are the relevant breaking changes so far:

  1. Required Node versions will be ^18 || >=20
  2. Hot-reloading of React components in files without a .[jt]sx extension is not really supported, unfortunately. This is the largest change. I added some workarounds in vite.config.mjs to allow compiling of JSX syntax in files without .[jt]sx extensions, so apps can at least start once they install this new version, but consumers will need to change their file extensions to get the full HMR behavior. This design by Vite is intended to save computations by not needing to run a JSX parser on files without JSX, so it's limited to .[jt]sx files. There are more notes inside the config file. I'm investigating easy ways like a codemod to run through a repo and update filenames of files containing JSX
  3. Removing react-scripts revealed a bug in @dhis2/cli-style -- it was missing a dependency that react-scripts filled. Consumers will need to upgrade @dhis2/cli-style to ^10.5.2
  4. The start script does not open the browser by default. This is the default Vite behavior, but I'm curious about input on this -- I like it, because opening the browser annoys me personally (since it doesn't work well on Firefox and Brave, the browsers I use), but maybe other people depend on it

Here are some other general notes:

  1. I tested typescript by converting some of the example app files to .tsx and it worked smoothly (albeit with many type errors)
  2. I took a rough benchmark to test the speed by building the simple example app, after bootstrapping the app shell and warming up the cache: 16s before, and 7.5s after. Pretty cool!
  3. Library building and unit tests are still working as they did before. I think we can build libraries with Vite in the future, but we might want to keep Jest tests as they are for backwards compatibility. Maybe we can introduce Vitest support alongside it, so can people can migrate, then we can eventually deprecate Jest support
  4. The Vite config currently lives in the shell, as if it were a standalone app. This works for starting and building the app, but in the future, it will probably need to get integrated into the CLI to handle dynamic things like plugin entrypoints, or Vite config extensions that platform consumers could define for their apps. This will also simplify the handling of environment variables that are passed to the app
  5. The Vite config has a .mjs extension to avoid a warning about the cjs package of Vite being deprecated
  6. The output dir in the build folder is now called ‘assets’ as the Rollup default instead of ‘static’, which should hopefully be a harmless implementation detail

Comment on lines +142 to +143
reporter.info('Compiling service worker...')
await compileServiceWorker({ config, paths, mode })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, CRA compiled the service worker in production builds. We already have our own compile function that we use for dev builds, so we use that here now

Comment on lines -20 to +17
const relative = normalizeExtension(path.relative(inputDir, source))
const relative = path.relative(inputDir, source)
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 was what broke JSX files in platform apps/libs: as an example, consider index.jsx has import App from 'App.jsx'. At compile time, App.jsx gets converted to App.js, but the import text in index is still from App.jsx (it doesn't get modified despite the filename change), and then an error is thrown that says "Can't find App.jsx"

@@ -82,12 +81,11 @@ exports.verifyEntrypoints = ({

const getEntrypointWrapper = async ({ entrypoint, paths }) => {
const relativeEntrypoint = entrypoint.replace(/^(\.\/)?src\//, '')
const outRelativeEntrypoint = normalizeExtension(relativeEntrypoint)
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 was also mishandling .jsx entrypoints

Comment on lines -90 to +88
.replace(/'.\/D2App\/app'/g, `'./D2App/${outRelativeEntrypoint}'`)
.replace(/'.\/D2App\/app\.jsx'/g, `'./D2App/${relativeEntrypoint}'`)
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 placeholder filename in the shell is now called app.jsx

Comment on lines -45 to +49
// Skip index.html, (plugin.html,) and `static` directory;
// CRA's workbox-webpack-plugin handles it smartly
globIgnores: [
'static/**/*',
paths.launchPath,
paths.pluginLaunchPath,
// skip moment locales -- they result in many network requests and
// slow down service worker installation
'**/moment-locales/*',
'**/*.map',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few changes in this file now that it's taking over from CRA

// (see https://regex101.com/r/z4Hy9k/1/ for RegEx details)
dontCacheBustURLsMatching: /\.[a-z0-9]{8}\.|\d+\.\d+\.\d+/,
// (see https://regex101.com/r/z4Hy9k/3/ for RegEx details)
dontCacheBustURLsMatching: /[.-][A-Za-z0-9-_]{8}\.|\d+\.\d+\.\d+/,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated for the chunks emitted from Vite's Rollup

// Precache all of the assets generated by your build process.
// Their URLs are injected into the manifest variable below.
// This variable must be present somewhere in your service worker file,
// even if you decide not to use precaching. See https://cra.link/PWA.
// Includes all built assets and index.html
// Injection point for the precache manifest from workbox-build,
// a manifest of app assets to fetch and cache upon SW installation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few things are cleaned up in this file now that our own compilation script is handling service workers instead of CRA's

Comment on lines -14 to -18
// Note that the CRA precache manifest files start with './'
// TODO: Make this extensible with a d2.config.js option
export const CRA_MANIFEST_EXCLUDE_PATTERNS = [
/^\.\/static\/js\/moment-locales\//,
]
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 was used to modify the manifest output by CRA; it's no longer needed

Comment on lines +58 to +59
<!-- The entrypoint for Vite -->
<script type="module" src="./src/index.jsx"></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.

Vite wants index.html to be here at the root directory level. This script tag is the entrypoint for the rest of the app

@@ -17,7 +17,7 @@
]
},
"devDependencies": {
"@dhis2/cli-style": "^10.4.3",
"@dhis2/cli-style": "^10.5.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -29,7 +29,7 @@
"@babel/preset-react": "^7.0.0",
"@babel/preset-typescript": "^7.6.0",
"@dhis2/app-shell": "11.2.0",
"@dhis2/cli-helpers-engine": "^3.2.0",
"@dhis2/cli-helpers-engine": "^3.2.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KaiVandivier KaiVandivier requested a review from a team May 8, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant