feat: 👷 Use vite instead of rollup #514

roninjin10 · 2023-03-27T15:03:19Z

Rather than opening an issue I figured I'd make a pr and see what everyone thought about this.

Problems

Packages are a mix of esm and cjs both in package module type and in build output type
Jest struggles with modern tooling like es modules. Esm is only supported experimentally
Jest wasn't working in this react package. We had a hack where we compiled the tests to js first
Jest is generally slow
Jest requires it's own seperate build process. You configure your normal rollup build, but then you have to configure a completely seperate version of doing similar thing in jest. This is hard to mantain
USers tools cannot properly code split react output since this package happened to only output cjs
Lack of a consistent pattern for monorepo/package management is the cause of some other issues I would suspect

Solution

Build outputs

I changed this react package to output both cjs and esm as two different build outputs
I included cjs build output as main in package.json and esm output as module. This will make it work seemlessly no matter what build tool user is using
I switched to vitest which is pretty close to the same api as jest but faster and supports modern tooling by default
I also switched from rollup to vite. Vite uses rollup under the hood and is a superset of rollup
This allows the build and tests to share the same config

packages/react/package.json

roninjin10 · 2023-03-27T15:12:22Z

packages/react/package.json

    "fast-deep-equal": "^3.1.3"
  },
  "devDependencies": {
    "@latticexyz/recs": "^1.41.0",
+    "@latticexyz/schema-type": "^1.41.0",
+    "@latticexyz/utils": "^1.41.0",


I added some peer deps of @lattice/recs that were missing

packages/react/src/useComponentValue.test.ts

roninjin10 · 2023-03-27T15:13:13Z

packages/react/vite.config.ts

+    environment: "jsdom",
+    globals: true,
+  },
+  plugins: [nodeResolve() as any, typescript(), commonjs(), peerDepsExternal(), pluginJson()],


These are the same rollup plugins being used before. We likely don't need all of them anymore vite likely handles some of this but included same plugins so this change is less likely to break anything

any way we can test for which ones we actually need and remove the rest? would be nice to slim this down as I don't know when we'll come back to this

Yes would be easy just need to remove them 1 at a time and see if they break or don't break. I'm guessing we don't need nodeResolve. I'm guessing we don't need commonjs(). I'm guessing we don't need pluginJson

I was right removed everything except typescript and peerDepsExternal

roninjin10 · 2023-03-27T15:13:46Z

packages/react/vite.config.ts

+  test: {
+    environment: "jsdom",
+    globals: true,
+  },


this config is the test config too which is very nice

Note how I didn't need to change any of the tests. This is because jest and vitest are almost the same api

roninjin10 · 2023-03-27T15:13:58Z

packages/react/vite.config.ts

+    lib: {
+      name: "@latticexyz/react",
+      entry: "src/index.ts",
+      formats: ["cjs", "es"],


Before we defaulted to only building cjs

roninjin10 · 2023-03-27T15:15:38Z

packages/react/package.json

+  "type": "module",
+  "types": "dist/index.d.ts",
+  "main": "dist/index.js",
+  "module": "dist/index.mjs",


The way this repo in general handles setting all this up is a bit of a mess. This is what build tools use to find the types, esm, or cjs. I would clean this up everywhere.

Currently we're pointing main to the typescript source in order to have instant updates during development, and then only set it to dist/index.js in the prerelease step. Down to change this if find an equally performant alternative to pointing to the ts source for development though

roninjin10 · 2023-03-27T15:16:01Z

packages/react/.eslintrc

-  "plugins": ["react", "react-hooks"]
+  "plugins": ["react", "react-hooks"],
+  "rules": {
+    "@typescript-eslint/consistent-type-imports": "error"


This is a good rule to always have turned on and it has an autofixer

can we separate this into its own PR and turn this on everywhere? or is this needed here for better builds?

Yea it should be seperate pr. This pr in general mostly exists just to try to solicit early feedback before I go about refactoring anything. I was going to open an issue but this pr was quick to whip up and I figured it would be better than an issue

removed from this pr I can open a seperate pr later this week to add it. I'll see if there are any other lint rules useful to add too while I'm at it. I will only add a lint rule if it has an autofixer and is useful

holic · 2023-03-27T15:56:07Z

ooc why did you resolve your own comments? makes it a little hard to have a conversation in them now that they're all collapsed

roninjin10 · 2023-03-27T16:08:30Z

@holic reopening any that conversations are happening in. Since I tend to leave tons of comments on all my prs that are mostly there to just provide extra context my general habit is to comment and then resolve it so the comment is there but the comment is also not getting in the way creating extra noise in the review

holic · 2023-03-27T16:14:27Z

@holic reopening any that conversations are happening in. Since I tend to leave tons of comments on all my prs that are mostly there to just provide extra context my general habit is to comment and then resolve it so the comment is there but the comment is also not getting in the way creating extra noise in the review

the context is helpful for us reviewing too, so I have a slight preference to leave em open!

packages/utils/package.json

remove rollup and jest config Remove acccidental dep revert eslint change for a seperate pr remove unused plugins remove rollup plugins from correct package.json

roninjin10 · 2023-03-27T16:32:50Z

packages/react/jest.config.js

-  preset: "ts-jest",
-  moduleNameMapper: {
-    // fix TS build issues
-    "^(..?/.*).js$": "$1",


Here can see jest wasn't working as intended

holic · 2023-04-12T21:12:38Z

moved this to a draft until we can rebase/merge main now that pnpm has landed

holic · 2023-05-04T17:01:46Z

we're moving to tsup, see #660

roninjin10 requested review from holic and alvrs as code owners March 27, 2023 15:03

roninjin10 commented Mar 27, 2023

View reviewed changes

packages/react/package.json Outdated Show resolved Hide resolved

roninjin10 commented Mar 27, 2023

View reviewed changes

packages/react/src/useComponentValue.test.ts Outdated Show resolved Hide resolved

roninjin10 commented Mar 27, 2023

View reviewed changes

packages/utils/package.json Outdated Show resolved Hide resolved

feat: Create POC for vite in react

980d8ec

remove rollup and jest config Remove acccidental dep revert eslint change for a seperate pr remove unused plugins remove rollup plugins from correct package.json

roninjin10 force-pushed the willc/vitest branch from cd10673 to 980d8ec Compare March 27, 2023 16:31

roninjin10 commented Mar 27, 2023

View reviewed changes

dk1a mentioned this pull request Apr 4, 2023

feat(plugins): create plugins package #584

Closed

dk1a mentioned this pull request Apr 12, 2023

build: yarn -> pnpm #591

Merged

4 tasks

holic marked this pull request as draft April 12, 2023 21:12

holic closed this May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 👷 Use vite instead of rollup #514

feat: 👷 Use vite instead of rollup #514

roninjin10 commented Mar 27, 2023 •

edited

Loading

roninjin10 Mar 27, 2023

roninjin10 Mar 27, 2023

holic Mar 27, 2023

roninjin10 Mar 27, 2023

roninjin10 Mar 27, 2023

roninjin10 Mar 27, 2023

roninjin10 Mar 27, 2023

roninjin10 Mar 27, 2023

roninjin10 Mar 27, 2023

alvrs Apr 12, 2023

roninjin10 Mar 27, 2023

holic Mar 27, 2023 •

edited

Loading

roninjin10 Mar 27, 2023

roninjin10 Mar 27, 2023

holic commented Mar 27, 2023

roninjin10 commented Mar 27, 2023

holic commented Mar 27, 2023

roninjin10 Mar 27, 2023

holic commented Apr 12, 2023 •

edited

Loading

holic commented May 4, 2023

feat: 👷 Use vite instead of rollup #514

feat: 👷 Use vite instead of rollup #514

Conversation

roninjin10 commented Mar 27, 2023 • edited Loading

Problems

Solution

Build outputs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic commented Mar 27, 2023

roninjin10 commented Mar 27, 2023

holic commented Mar 27, 2023

Choose a reason for hiding this comment

holic commented Apr 12, 2023 • edited Loading

holic commented May 4, 2023

roninjin10 commented Mar 27, 2023 •

edited

Loading

holic Mar 27, 2023 •

edited

Loading

holic commented Apr 12, 2023 •

edited

Loading