build: use esbuild #343

aulneau · 2021-03-26T12:28:02Z

Hi there!

I made a PR for jotai a few days ago updating the build tooling to use esbuild as the typescript transpiler and @dai-shi requested I make PRs for zustand and valtio, so here we are! The main reason for this change is because of how much faster esbuild is at compiling javascript from typescript:

Before
✨ Done in 25.26s.

After
✨ Done in 6.80s.

There are also some very minor bundle size changes with this update.

General overview of changes

added esbuild and rollup-plugin-esbuild
replaced the typescript plugin in createESMConfig, createCommonJSConfig, and createIIFEConfig to use esbuild
added id.includes('@babel/runtime') to the externals check per this recommendation
updated the node version used in the codesandbox ci to 12 (due to esbuild requirement)

codesandbox-ci · 2021-03-26T12:31:48Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f66f187:

Sandbox	Source
React	Configuration
React Typescript	Configuration

aulneau · 2021-03-26T12:36:20Z

@dai-shi as I was comparing the packaging of these three packages, zustand stands out of the convention where you're bundling esm as a separate module, and defaulting to cjs as the main export. After this I saw this PR: #336. Curious how you want to move forward?

dai-shi · 2021-03-26T13:00:09Z

Yes, let's change it.
It's a breaking change for index.cjs users #336 (comment), but i'm going with just minor update with breaking change notice in the release page, as this use case is not very common.

dai-shi · 2021-03-26T13:00:44Z

Same apply here with the comment in valtio: pmndrs/valtio#129 (comment)

rollup.config.js

aulneau · 2021-03-26T13:09:07Z

@dai-shi I haven't seen this kind of thing before: createIIFEConfig, do you want it in both jotai/valtio?

dai-shi · 2021-03-26T13:12:16Z

Hm, I think we can drop iife build in zustand. I don't know who is using in which case.
Wonder if @drcmda or @JeremyRH have an idea?

package.json

aulneau · 2021-03-26T13:26:04Z

There is one new issue that I don't quite understand: https://rollupjs.org/guide/en/#error-this-is-undefined

aulneau · 2021-03-26T13:36:33Z

@dai-shi I think it's related to this rollup/rollup#1518 (comment)

here: https://github.com/pmndrs/zustand/blob/master/src/middleware.ts#L218 and https://github.com/pmndrs/zustand/blob/master/src/middleware.ts#L241

src/middleware.ts

dai-shi · 2021-03-26T13:46:04Z

Is it happening only for cjs build (esbuild + babel), or also module build (esbuild only)? Or, am I missing something totally.

aulneau · 2021-03-26T13:47:30Z

Is it happening only for cjs build (esbuild + babel), or also module build (esbuild only)? Or, am I missing something totally.

Just for the cjs bundle. It seems to be an issue of the conversion down to es5, but #343 (comment) fixes it

dai-shi · 2021-03-26T14:28:28Z

@aulneau Thanks for your investigation.
Actually, I would prefer a solution without changing the src, because it affects the module bundle.
Would you try moduleContext or the custom onwarn? I have no idea what to specify for moduleContext though, null?

aulneau · 2021-03-26T15:13:10Z

Actually, I would prefer a solution without changing the src, because it affects the module bundle.

I am curious though, I think if we do not make this change, the resulting cjs bundle will not work if I understand what the issue is.

Can I ask why you would prefer to keep arrow functions over non? isn't the result the same?

dai-shi · 2021-03-26T15:30:55Z

I think if we do not make this change, the resulting cjs bundle will not work if I understand what the issue is.

I didn't take like that... It just seems a potential warning. I will read it again.

isn't the result the same?

For the module bundle, arrow functions are not transpiled. I think the current code looks just right and don't want to prohibit unbound arrow functions for the future.

aulneau · 2021-03-26T15:55:12Z

I didn't take like that... It just seems a potential warning. I will read it again.

I see, if it's just a warning then that should be fine.

dai-shi · 2021-03-27T09:13:04Z

rollup/rollup#1518 (comment)

It's a warning because sometimes you can ignore it and sometimes you can't.

I'm assuming this is a warning not for us.
Please try other options without modifying src:

rollup-plugin-async (this is a totally different solution, but it might turn to be good.)
moduleContext to null
custom onwarn to ignore 'THIS_IS_UNDEFINED'

@aulneau once this is done, let's ask someone to help for trial in discord.

aulneau · 2021-03-27T14:36:34Z

@dai-shi sure thing!

Question for you around the middleware.ts file. I see here that the promise is used in setItem, however here and here the promise is ignored. Is that intentional? If so, do you want to add void before the ignored promises?

dai-shi · 2021-03-27T14:45:52Z

Question for you around the middleware.ts file. I see here that the promise is used in setItem, however here and here the promise is ignored. Is that intentional? If so, do you want to add void before the ignored promises?

I think it's intentional. @AnatoleLucet can confirm.

I don't mind adding void as you suggested in #343 (comment).
So, this is unrelated with THIS_IS_UNDEFINED thing, right?

AnatoleLucet · 2021-03-27T14:52:48Z

Question for you around the middleware.ts file. I see here that the promise is used in setItem, however here and here the promise is ignored. Is that intentional? If so, do you want to add void before the ignored promises?

I think it's intentional. @AnatoleLucet can confirm.

I don't mind adding void as you suggested in #343 (comment).
So, this is unrelated with THIS_IS_UNDEFINED thing, right?

Yes, it's intentional. I don't mind adding a void either.

aulneau · 2021-03-27T14:59:08Z

So, this is unrelated with THIS_IS_UNDEFINED thing, right?

Yep, just something I've noticed.

So everything seems to be working now. I realized we actually don't need to use either @rollup/plugin-typescript or rollup-plugin-esbuild on the cjs output as we're using babel and it seems to handle it all perfectly fine! Seems to have cut another half a second off the total time :) I think this is ready for folks to test! It'd be good to get some esm based apps (Eg using vite or snowpack) and some cjs apps (maybe a next.js app or something) to ensure nothing is broken!

package.json

rollup.config.js

JeremyRH · 2021-03-28T20:28:30Z

@aulneau Awesome job with this PR. Using rollup-plugin-esbuild to parse TypeScript and concurrently to run the builds in parallel is great. I suspect a lot of the build time improvements are due to running them in parallel.

In the future, we should look at using esbuild directly in place of rollup. It doesn't support ES5 output but if we plan on removing the iife bundle, do we need to output ES5?

dai-shi · 2021-03-28T22:21:49Z

It doesn't support ES5 output but if we plan on removing the iife bundle, do we need to output ES5?

Both iife and cjs used to target ie11, and it doesn't change for cjs in this pr. We still use babel for cjs.

dai-shi

Looks good. Thanks for your work!

rollup.config.js

dai-shi · 2021-03-31T03:31:57Z

rollup.config.js

-    ]
-    config.babelHelpers = 'runtime'
+    config.plugins = [...config.plugins, '@babel/plugin-transform-regenerator']
+    config.babelHelpers = 'bundled'


Let me check how much this would affect the bundle size.

'runtime' 'bundled' diff

index.js 3760 3561 -199

middleware.js 8720 9147 +427

shallow.js 620 620 0

vanilla.js 1798 1798 0

Okay, this is more or less what I expected.
We will probably be refactoring persist in middleware.ts without async/await, so the diff is likely to be reduced in the future.
Let's go with bundled as cjs build is kind of fallback.

aulneau mentioned this pull request Mar 26, 2021

build: use esbuild plugin pmndrs/valtio#129

Merged

aulneau force-pushed the build/esbuild branch from 0157e06 to f2bd14b Compare March 26, 2021 12:30

aulneau mentioned this pull request Mar 26, 2021

build: use esbuild and build concurrently pmndrs/jotai#378

Merged

dai-shi reviewed Mar 26, 2021

View reviewed changes

rollup.config.js Outdated Show resolved Hide resolved

aulneau requested a review from dai-shi March 26, 2021 13:20

dai-shi reviewed Mar 26, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

aulneau commented Mar 26, 2021

View reviewed changes

src/middleware.ts Outdated Show resolved Hide resolved

aulneau commented Mar 26, 2021

View reviewed changes

src/middleware.ts Show resolved Hide resolved

aulneau force-pushed the build/esbuild branch from 593be2b to a7e2f12 Compare March 27, 2021 14:53

aulneau force-pushed the build/esbuild branch from a7e2f12 to 117ade4 Compare March 27, 2021 15:00

dai-shi reviewed Mar 27, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

dai-shi reviewed Mar 27, 2021

View reviewed changes

rollup.config.js Show resolved Hide resolved

aulneau force-pushed the build/esbuild branch from 9339d7c to f92c81a Compare March 28, 2021 13:59

aulneau requested a review from dai-shi March 28, 2021 15:40

dai-shi approved these changes Mar 28, 2021

View reviewed changes

dai-shi reviewed Mar 29, 2021

View reviewed changes

rollup.config.js Show resolved Hide resolved

aulneau force-pushed the build/esbuild branch 2 times, most recently from 7a26795 to a234671 Compare March 31, 2021 00:56

build: use esbuild

f66f187

aulneau force-pushed the build/esbuild branch from a234671 to f66f187 Compare March 31, 2021 01:02

dai-shi reviewed Mar 31, 2021

View reviewed changes

dai-shi merged commit 77334ec into pmndrs:master Apr 1, 2021

This was referenced Apr 1, 2021

fix: types for index.cjs #337

Closed

fix: revert babel runtime #352

Merged

dai-shi mentioned this pull request Apr 9, 2021

Create a just ESM version #336

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: use esbuild #343

build: use esbuild #343

aulneau commented Mar 26, 2021 •

edited

Loading

codesandbox-ci bot commented Mar 26, 2021 •

edited

Loading

aulneau commented Mar 26, 2021

dai-shi commented Mar 26, 2021

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021 •

edited

Loading

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021

aulneau commented Mar 26, 2021

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021

dai-shi commented Mar 27, 2021

aulneau commented Mar 27, 2021

dai-shi commented Mar 27, 2021

AnatoleLucet commented Mar 27, 2021

aulneau commented Mar 27, 2021

JeremyRH commented Mar 28, 2021 •

edited

Loading

dai-shi commented Mar 28, 2021

dai-shi left a comment

dai-shi Mar 31, 2021

dai-shi Apr 1, 2021

	`'runtime'`	`'bundled'`	diff
index.js	3760	3561	-199
middleware.js	8720	9147	+427
shallow.js	620	620	0
vanilla.js	1798	1798	0

build: use esbuild #343

build: use esbuild #343

Conversation

aulneau commented Mar 26, 2021 • edited Loading

General overview of changes

codesandbox-ci bot commented Mar 26, 2021 • edited Loading

aulneau commented Mar 26, 2021

dai-shi commented Mar 26, 2021

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021 • edited Loading

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021

aulneau commented Mar 26, 2021

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021

dai-shi commented Mar 26, 2021

aulneau commented Mar 26, 2021

dai-shi commented Mar 27, 2021

aulneau commented Mar 27, 2021

dai-shi commented Mar 27, 2021

AnatoleLucet commented Mar 27, 2021

aulneau commented Mar 27, 2021

JeremyRH commented Mar 28, 2021 • edited Loading

dai-shi commented Mar 28, 2021

dai-shi left a comment

Choose a reason for hiding this comment

dai-shi Mar 31, 2021

Choose a reason for hiding this comment

dai-shi Apr 1, 2021

Choose a reason for hiding this comment

aulneau commented Mar 26, 2021 •

edited

Loading

codesandbox-ci bot commented Mar 26, 2021 •

edited

Loading

aulneau commented Mar 26, 2021 •

edited

Loading

JeremyRH commented Mar 28, 2021 •

edited

Loading