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.

Sign up for GitHub

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

Jump to bottom

replace `react-scripts` with `vite` #1314

Closed

eventualbuddha opened this issue Dec 13, 2021 · 2 comments

Assignees

Labels

tech-improvement

Collaborator

eventualbuddha commented Dec 13, 2021 •

edited by carolinemodic

Loading

Why?

It's fast. Vite (pronounced "veet") uses esbuild, which is potentially orders of magnitude faster than with babel + webpack. Add TypeScript type checking on top of that and the difference is even more huge.
It's lightweight. CRA includes the kitchen sink and battery-includes everything. This is good for getting started, but bad for upgrades and customization. Vite includes only what you need for HMR development and fast builds. Testing/linting is separate.

The Plan

Proposal
I'd like to evaluate it for use with vxsuite. I've done some initial exploration and it seems doable, but will require some restructuring. For example:

probably shouldn't use NodeJS builtins polyfills like for assert or crypto. there are ways to polyfill but we hopefully won't need to
stop using TS project references, a la https://github.com/mmazzarolo/tangerine-monorepo
more that I haven't uncovered yet

The text was updated successfully, but these errors were encountered:

eventualbuddha added a commit that referenced this issue


          refactor(utils): bundle simple assert

30e271e

Rather than relying on the NodeJS `assert` module or a polyfill in the browser, we can use this simple `assert` function to achieve the same effect. This removes one of the roadblocks toward using a non-webpack dev/build system.

Refs #1314

eventualbuddha mentioned this issue

refactor(utils): bundle simple assert #1315

Merged

eventualbuddha added a commit that referenced this issue


          feat(codemods): add use_utils_assert codemod

b38c31e

Transforms uses of `assert` that can be converted automatically to use `@votingworks/utils` instead.

Refs #1314

eventualbuddha mentioned this issue

feat(codemods): add use_utils_assert codemod #1316

Merged

eventualbuddha added a commit that referenced this issue


          feat(codemods): add use_utils_assert codemod

b4cfa80

Transforms uses of `assert` that can be converted automatically to use `@votingworks/utils` instead.

Refs #1314

eventualbuddha added a commit that referenced this issue


          feat(codemods): add use_utils_assert codemod

18310af

Transforms uses of `assert` that can be converted automatically to use `@votingworks/utils` instead.

Refs #1314

eventualbuddha added a commit that referenced this issue


          refactor(all): use @votingworks/utils assert

c53f9c3

This was done by the codemod from #1316.

Refs #1314

eventualbuddha added a commit that referenced this issue


          style(all): fix formatting after codemod

18d5b35

Refs #1314

eventualbuddha added a commit that referenced this issue


          fix(all): fix issues with assert calls

6594b37

Our `assert` doesn't have helpers like `equal`, so just pass a truthy value. Also, fix issues flagged by linting.

Refs #1314

eventualbuddha added a commit that referenced this issue


          style(all): fix formatting after codemod

660b468

Refs #1314

eventualbuddha added a commit that referenced this issue


          fix(all): fix issues with assert calls

ae04779

Our `assert` doesn't have helpers like `equal`, so just pass a truthy value. Also, fix issues flagged by linting.

Refs #1314

eventualbuddha mentioned this issue

refactor(all): use @votingworks/utils assert #1317

Merged

Collaborator Author

eventualbuddha commented Apr 5, 2022

#1669 was related to this work.

eventualbuddha added a commit that referenced this issue


          refactor(all): replace crypto with npm deps

f246982

Refs #1314

In several packages we import the `crypto` NodeJS built-in library. This works as expected in NodeJS, but for the browser we rely on CRA via webpack to bundle a polyfill. To reduce our reliance on CRA and webpack, this commit switches to explicit polyfills from npm.

In all but the `lsd` module (because it's NodeJS-only), replaces uses of `crypto` with `randombytes` (which uses `crypto` in NodeJS and `globalThis.crypto` in the browser) or `js-sha256` (which likewise uses `crypto` in NodeJS and its own implementation in the browser).

eventualbuddha added a commit that referenced this issue


          refactor(all): replace assert with internal one

b52a096

Refs #1314

In several packages we import the `assert` NodeJS built-in library. This works as expected in NodeJS, but for the browser we rely on CRA via webpack to bundle a polyfill. To reduce our reliance on CRA and webpack, this commit switches to our own `assert` from `@votingworks/utils`.

eventualbuddha added a commit that referenced this issue


          chore(bmd): remove @types/node package

a878026

As we move toward #1314, this will make it harder to introduce a dependency on the built-in NodeJS packages.

eventualbuddha mentioned this issue

refactor(all): replace some NodeJS built-in modules #1675

Merged

4 tasks

eventualbuddha added a commit that referenced this issue


          refactor(all): replace crypto with npm deps

Refs #1314

In several packages we import the `crypto` NodeJS built-in library. This works as expected in NodeJS, but for the browser we rely on CRA via webpack to bundle a polyfill. To reduce our reliance on CRA and webpack, this commit switches to explicit polyfills from npm.

In all but the `lsd` module (because it's NodeJS-only), replaces uses of `crypto` with `randombytes` (which uses `crypto` in NodeJS and `globalThis.crypto` in the browser) or `js-sha256` (which likewise uses `crypto` in NodeJS and its own implementation in the browser).

eventualbuddha added a commit that referenced this issue


          refactor(all): replace assert with internal one

b742fc6

Refs #1314

In several packages we import the `assert` NodeJS built-in library. This works as expected in NodeJS, but for the browser we rely on CRA via webpack to bundle a polyfill. To reduce our reliance on CRA and webpack, this commit switches to our own `assert` from `@votingworks/utils`.

eventualbuddha added a commit that referenced this issue


          chore(bmd): remove @types/node package

1bb0767

As we move toward #1314, this will make it harder to introduce a dependency on the built-in NodeJS packages.

eventualbuddha added a commit that referenced this issue


          refactor(all): replace crypto with npm deps

74f0ef5

Refs #1314

In several packages we import the `crypto` NodeJS built-in library. This works as expected in NodeJS, but for the browser we rely on CRA via webpack to bundle a polyfill. To reduce our reliance on CRA and webpack, this commit switches to explicit polyfills from npm.

In all but the `lsd` module (because it's NodeJS-only), replaces uses of `crypto` with `randombytes` (which uses `crypto` in NodeJS and `globalThis.crypto` in the browser) or `js-sha256` (which likewise uses `crypto` in NodeJS and its own implementation in the browser).

eventualbuddha added a commit that referenced this issue


          refactor(all): replace assert with internal one

bfa8bae

Refs #1314

In several packages we import the `assert` NodeJS built-in library. This works as expected in NodeJS, but for the browser we rely on CRA via webpack to bundle a polyfill. To reduce our reliance on CRA and webpack, this commit switches to our own `assert` from `@votingworks/utils`.

eventualbuddha added a commit that referenced this issue


          chore(bmd): remove @types/node package

38dbfaf

As we move toward #1314, this will make it harder to introduce a dependency on the built-in NodeJS packages.

eventualbuddha added a commit that referenced this issue


          refactor(all): replace crypto with npm deps

30e57f0

Refs #1314

In several packages we import the `crypto` NodeJS built-in library. This works as expected in NodeJS, but for the browser we rely on CRA via webpack to bundle a polyfill. To reduce our reliance on CRA and webpack, this commit switches to explicit polyfills from npm.

In all but the `lsd` module (because it's NodeJS-only), replaces uses of `crypto` with `randombytes` (which uses `crypto` in NodeJS and `globalThis.crypto` in the browser) or `js-sha256` (which likewise uses `crypto` in NodeJS and its own implementation in the browser).

eventualbuddha added a commit that referenced this issue


          refactor(all): replace assert with internal one

910ce4a

Refs #1314

In several packages we import the `assert` NodeJS built-in library. This works as expected in NodeJS, but for the browser we rely on CRA via webpack to bundle a polyfill. To reduce our reliance on CRA and webpack, this commit switches to our own `assert` from `@votingworks/utils`.

eventualbuddha added a commit that referenced this issue


          chore(bmd): remove @types/node package

da59798

As we move toward #1314, this will make it harder to introduce a dependency on the built-in NodeJS packages.

eventualbuddha mentioned this issue

upgrade react-scripts/CRA to v5 #1786

Closed

2 tasks

eventualbuddha mentioned this issue

build: replace react-scripts with vite #1831

Closed

41 tasks

eventualbuddha added a commit that referenced this issue


          refactor(types): split out API types

6fb706d

Importing from a sub-path of a package doesn't play well with some tooling, especially when those packages are build-less as when using vite.

Refs #1314

eventualbuddha mentioned this issue

refactor(types): split out API types #1840

Merged

4 tasks

eventualbuddha added a commit that referenced this issue


          refactor(types): split out API types

a0a0a57

Importing from a sub-path of a package doesn't play well with some tooling, especially when those packages are build-less as when using vite.

Refs #1314

eventualbuddha added a commit that referenced this issue


          refactor(types): split out API types

f3a5110

Importing from a sub-path of a package doesn't play well with some tooling, especially when those packages are build-less as when using vite.

Refs #1314

eventualbuddha added a commit that referenced this issue


          refactor(types): split out API types

471c613

Importing from a sub-path of a package doesn't play well with some tooling, especially when those packages are build-less as when using vite.

Refs #1314

eventualbuddha mentioned this issue

chore(deps): update to @testing-library/react v12 #1848

Merged

4 tasks

carolinemodic assigned eventualbuddha

carolinemodic added the tech-improvement label

eventualbuddha added a commit that referenced this issue


          chore(dev-deps): standardize @types/react version

da24a06

Refs #1314

Switching to a buildless system will mean that TypeScript will see deeper into monorepo dependencies like `/ui` and their `@types/react` dependencies. This means we want the versions to match to avoid incompatibilities when e.g. type checking `bmd` or another frontend.

This was referenced May 24, 2022

chore(dev-deps): standardize @types/react version #1875

Merged

fix(ui): correct setResultsReportingUrl race condition #1884

Merged

eventualbuddha added a commit that referenced this issue


          chore(frontends): remove some CRA cruft

Refs #1314

We don't use service workers and probably never will. We don't need the types provided by `react-scripts`.

eventualbuddha added a commit that referenced this issue


          chore(frontends): remove some CRA cruft

5c8228d

Refs #1314

We don't use service workers and probably never will. We don't need the types provided by `react-scripts`.

eventualbuddha added a commit that referenced this issue


          chore(frontends): remove some CRA cruft

8ca961d

Refs #1314

We don't use service workers and probably never will. We don't need the types provided by `react-scripts`.

This was referenced Jun 1, 2022

build(bmd): eject CRA app #1904

Merged

build(bas): eject & add vite option #1905

Merged

build(bmd): add vite as dev & build option #1906

Merged

build(bsd): add vite as a dev & build option #1908

Merged

build(all): enable isolatedModules #1909

Merged

build(em): use vite for dev & build #1933

Merged

eventualbuddha mentioned this issue

chore(test-utils): enable noUncheckedIndexedAccess #1950

Merged

4 tasks

eventualbuddha changed the title ~~test using Vite for dev+build instead of CRA~~ replace react-scripts with vite

eventualbuddha added a commit that referenced this issue


          build(ps): switch from CRA to vite

b791abc

Refs #1314

eventualbuddha mentioned this issue

build(ps): switch from CRA to vite #2018

Merged

4 tasks

eventualbuddha added a commit that referenced this issue


          build(ps): switch from CRA to vite

ce85381

Refs #1314

eventualbuddha added a commit that referenced this issue


          build(bsd): switch from CRA to vite

02786eb

Refs #1314

eventualbuddha added a commit that referenced this issue


          test(bsd): switch from CRA to vite

ab549e8

Refs #1314

eventualbuddha mentioned this issue

test(bsd): switch from CRA to vite #2019

Merged

4 tasks

Collaborator Author

eventualbuddha commented Aug 17, 2022

I'm going to count this as done. We no longer use react-scripts, but we do still have some of the leftovers from ejecting. We can clean that stuff up later.

eventualbuddha closed this as completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment