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

chore(build): move tsdx to esbuild #957

Merged
merged 22 commits into from
Apr 1, 2021

Conversation

hardfist
Copy link
Contributor

just try what esbuild cann't be done for author libraries

@netlify
Copy link

netlify bot commented Mar 29, 2021

Deploy preview for redux-starter-kit-docs ready!

Built with commit 7c6f46d

https://deploy-preview-957--redux-starter-kit-docs.netlify.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 29, 2021

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 7c6f46d:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

@markerikson
Copy link
Collaborator

Hey, thanks for putting this together.

Unfortunately, we do still currently include a UMD build in our published packages, and I don't think ESBuild supports that at all.

@hardfist
Copy link
Contributor Author

Hey, thanks for putting this together.

Unfortunately, we do still currently include a UMD build in our published packages, and I don't think ESBuild supports that at all.

done!, I just need to convert bundled esm to umd by rollup

@markerikson
Copy link
Collaborator

So there's a couple other nuances to be aware of with our UMD builds.

We currently output both a dev and prod UMD build.

Not only is the prod UMD build minified, but we need to strip out a couple bits of code from getDefaultMiddleware.ts that are dev-mode only. Currently, we use a rollup-plugin-strip-code to do that.

@hardfist
Copy link
Contributor Author

So there's a couple other nuances to be aware of with our UMD builds.

We currently output both a dev and prod UMD build.

Not only is the prod UMD build minified, but we need to strip out a couple bits of code from getDefaultMiddleware.ts that are dev-mode only. Currently, we use a rollup-plugin-strip-code to do that.

only form umd production ? what about esm&cjs production?

@markerikson
Copy link
Collaborator

Uh... just UMD, I think? :) It's getDefaultMiddleware.ts, specifically.

That file's gone through a few different changes over time - you can see the changes in the git history.

@hardfist hardfist force-pushed the move-to-esbuild branch 2 times, most recently from e671753 to 8baad0f Compare March 29, 2021 03:43
@hardfist
Copy link
Contributor Author

Uh... just UMD, I think? :) It's getDefaultMiddleware.ts, specifically.

That file's gone through a few different changes over time - you can see the changes in the git history.

I use esbuild plugin to handle this, I think it's ok now, but I do not know how to test

scripts/build.js Outdated
const source = await fs.readFileSync(args.path, 'utf-8')
const defaultPattern = /\/\* PROD_START_REMOVE_UMD[\s\S]*?\/\* PROD_STOP_REMOVE_UMD \*\//g
const code = source.replace(defaultPattern, '')
// if (sourceMap !== false && sourcemap !== false) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to handle sourcemap

@markerikson
Copy link
Collaborator

markerikson commented Mar 29, 2021

SO FYI, I think the "replace chunk of code" behavior could be accomplished using ESBuild's "define" replacement capability.

Specifically, if we set up the definitions such that PROD_START_REMOVE_UMD */ and /* PROD_STOP_REMOVE_UMD were replaced with nothing, that would leave these code blocks commented out.

To test this behavior, we'd want to do some manual searching in the production UMD file to see if any contents from the immutability middleware are in there.

(Side note: it actually looks like the current getDefaultMiddleware.ts is actually a bit buggy in that respect - we really should be cutting out the serializability middleware in prod UMD builds using that same trick, and we're currently leaving it in.)

Also: it looks like the use of tsc is outputting a bunch of individual JS files to dist. Playing with your branch locally, I think we want:

"emitDeclarationOnly": true,

@markerikson
Copy link
Collaborator

So there's one other point of concern here.

I believe ESBuild only outputs "modern" JS ( evanw/esbuild#297 ). Right now, our CommonJS and ESM builds are back-compiled to cover ES5 and IE11, as far as I know.

I'm all in favor of dropping IE11/ES5-only support in general, but I'm not sure we can pull the trigger on that just yet. I don't know what the right semver rules are for that sort of thing.

@hardfist
Copy link
Contributor Author

i check the code length to ensure works,and check the require result of umd bundle

@hardfist
Copy link
Contributor Author

So there's one other point of concern here.

I believe ESBuild only outputs "modern" JS ( evanw/esbuild#297 ). Right now, our CommonJS and ESM builds are back-compiled to cover ES5 and IE11, as far as I know.

I'm all in favor of dropping IE11/ES5-only support in general, but I'm not sure we can pull the trigger on that just yet. I don't know what the right semver rules are for that sort of thing.

i normally using ts transpilemodule to transform esbuild bundle result to target es5,if you prefer i can change this

@markerikson
Copy link
Collaborator

Yeah, that was gonna be my next train of thought - transpiling the built output itself in order to enable running okay in IE11, vs transpiling the input files straight to ES5.

So here's two other things that would be useful to sort out.

First, right now TSDX is generating three files for CJS: redux-toolkit.cjs.develeopment.js, redux-toolkit.cjs.production.js, and an index.js file that conditionally imports one of those two:

https://unpkg.com/browse/@reduxjs/[email protected]/dist/

Right now this PR is generating the dev and prod files, but the dev file is missing the .development portion of the name, and we don't have the index.js file being generated.

Also: part of the reason why we're wanting to revamp the build system is that we're going to need to support additional entry points. In particular, we're going to be adding support for:

import {thing1} from "@reduxjs/toolkit/query"
import {thing2} from "@reduxjs/toolkit/query/react"

Any chance you could put together an example of how that might work here?

btw, really do appreciate you working with me on fleshing out this idea :) I was skeptical at first, but it does seem like this could work.

@hardfist
Copy link
Contributor Author

add additional entry points seems to need use NodeJS 12.7+ package exports, which typescript does not supports yet, microsoft/TypeScript#33079, and package subpath exports is actually an breaking change

@markerikson
Copy link
Collaborator

Hmm. We seem to have something working for multiple entry points over in the RTK Query repo, although admittedly we haven't published a real version with that build config - just the auto builds per PR.

@hardfist
Copy link
Contributor Author

add index.js entry points, down-level to es5, remove extra .js files

@hardfist
Copy link
Contributor Author

Hmm. We seem to have something working for multiple entry points over in the RTK Query repo, although admittedly we haven't published a real version with that build config - just the auto builds per PR.

I check the RTK query docs, didn't find any docs about additional entry points, could you give me an example, so I can do the same things here.

scripts/build.js Outdated
}js`,
write: false,
target: 'es2015',
minify: env === 'production',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo sourcemap merge

@markerikson
Copy link
Collaborator

The ultimate plan here is to have three entry points:

  • "@reduxjs/toolkit": standard RTK APIs ( configureStore, createSlice )
  • "@reduxjs/toolkit/query": RTK Query UI-agnostic version of createApi plus utils
  • "@reduxjs/toolkit/query/react": React-specific version of createApi that auto-generates React hooks too

You can see some of that config here:

https://github.com/rtk-incubator/rtk-query/blob/783a2ab7bfa6c966bffa86e17fe55a7b2f250c7e/rollup.config.js

and here's a CI build from rtk-incubator/rtk-query#177 so you can see the rough build output:

rtk-query-pr-177.zip

We'll need something sorta like that in order to release 1.6 the way we want to, but we're open to exactly how that gets done.

@hardfist
Copy link
Contributor Author

The ultimate plan here is to have three entry points:

  • "@reduxjs/toolkit": standard RTK APIs ( configureStore, createSlice )
  • "@reduxjs/toolkit/query": RTK Query UI-agnostic version of createApi plus utils
  • "@reduxjs/toolkit/query/react": React-specific version of createApi that auto-generates React hooks too

You can see some of that config here:

https://github.com/rtk-incubator/rtk-query/blob/783a2ab7bfa6c966bffa86e17fe55a7b2f250c7e/rollup.config.js

and here's a CI build from rtk-incubator/rtk-query#177 so you can see the rough build output:

rtk-query-pr-177.zip

We'll need something sorta like that in order to release 1.6 the way we want to, but we're open to exactly how that gets done.

put an package in another package ,clever idea, I will try this method

@hardfist
Copy link
Contributor Author

@markerikson it seems that query && query/react is in another repo, then How should I put the repo code here, so you want to use monorepo to put rtk/query code into RTK?

@markerikson
Copy link
Collaborator

We'll be migrating the RTK Query code over into this repo in the near future, but it's not ready yet. For now, could you temporarily just add a couple dummy source files and use those as the source for two new subpackages, just to see if we can get this to work?

Ideally, one subpackage would export a plain JS function, and the other subpackage would export both that first function and a custom React hook, because that is what we'll be doing later.

@hardfist
Copy link
Contributor Author

yes, exports definitely will break code under nodejs, but I'm little confused, is it really necesary to split file to index.js|xxx.development|xxx.production other than just ship an index.js which contains development and production code, since bundler will eventually treeshake the unused code according to process.env.NODE_ENV, and node.js project seems don't care so much about the size

@markerikson
Copy link
Collaborator

markerikson commented Mar 31, 2021

So in theory, bundlers should be picking up the .esm file, with its process.env values still embedded.

CJS is a fallback for Node. The split-file convention is something React has been using for a while, so we have too.

The .modern build is ESM, but with modern ES2017 syntax.

However, per #652 , files with process.env don't work in a browser. So, I added .modern.development and .modern.production files to support loading those in a browser directly (via Unpkg or similar).

@markerikson
Copy link
Collaborator

markerikson commented Mar 31, 2021

@hardfist okay, I was able to install the CSB-built package into a local CRA project, but there's something buggy about the imports/exports in redux-toolkit.esm.js:

Failed to compile.

./node_modules/@reduxjs/toolkit/dist/redux-toolkit.esm.js
Attempted import error: 'Draft' is not exported from 'immer'.

I see two sets of exports in that file. One is around like 75, like this:

import { enableES5 } from "immer";
export * from "redux";
import { default as default2, Draft as Draft2, current as current2, freeze, original, isDraft as isDraft4 } from "immer";
import { createSelector as createSelector2, Selector, OutputParametricSelector, OutputSelector, ParametricSelector } from "reselect";
// src/createDraftSafeSelector.ts
import { current, isDraft } from "immer";
import { createSelector } from "reselect";

Notice that a bunch of those are actually TS types, and this file should not be exporting those.

then, at the bottom, there's:

export { Draft2 as Draft, MiddlewareArray, OutputParametricSelector, OutputSelector, ParametricSelector, Selector, 
ThunkAction, ThunkDispatch, configureStore, createAction, createAsyncThunk, createDraftSafeSelector, 
createEntityAdapter, createImmutableStateInvariantMiddleware, default2 as createNextState, createReducer, 
createSelector2 as createSelector, createSerializableStateInvariantMiddleware, createSlice, current2 as current, 
findNonSerializableValue, freeze, getDefaultMiddleware, getType, isAllOf, isAnyOf, isAsyncThunkAction, 
isDraft4 as isDraft, isFulfilled, isImmutableDefault, isPending, isPlain, isPlainObject, isRejected, 
isRejectedWithValue, nanoid, original, unwrapResult };

which is A) duplicate, and B) still exporting a bunch of TS types that should not be exported.

It's getting late here and I gotta head to bed. Can you figure out what's going on there and fix it? Thank you!

@hardfist
Copy link
Contributor Author

hardfist commented Mar 31, 2021

haha,react doesn't support esm yet, so the bundle actually use cjs version of react, the esm split make sense to me now, since to support bundleness scenario

@hardfist
Copy link
Contributor Author

I will check it , I just get up

@markerikson
Copy link
Collaborator

I DEMAND THAT YOU FIX THIS NOW, BEFORE BREAKFAST!!!

:)

(seriously, again, appreciate all your efforts on this!)

@hardfist
Copy link
Contributor Author

hardfist commented Mar 31, 2021

It's kind of tricky here, since esbuild does not support shakes implicit type import|export, so we can change all type export to explicit type export or waiting esbuild to support this feature.
and the duplicate export seems not a problem(even thou it's kind of nasty here), since it exports different things here.

@markerikson
Copy link
Collaborator

I'm fine with changing whatever type import/export declarations we need to make this work - please go ahead!

@hardfist
Copy link
Contributor Author

woops, it seems that api-extractor doesn't supports type export yet

@hardfist
Copy link
Contributor Author

I currently disable api-extractor and ship all dts files instead, and it passes the redux-typescript cra template test. api-extractor seems needs some hack.

@hardfist
Copy link
Contributor Author

after I upgrade api-extractor to latest version, it works now

@hardfist
Copy link
Contributor Author

Oh, I don't know you have to support typescript version below 3.8 which doesn't support type export|import, can we drop support for this typescript version ?

@phryneas
Copy link
Member

Given that by now 4.2 is out, that would still fall under "support the last 5 TS versions", so I'd be fine with it.

@markerikson
Copy link
Collaborator

markerikson commented Mar 31, 2021

Yeah, we can drop TS <3.8 for RTK 1.6

I thought of another thing I'd like us to look at as well, although it doesn't strictly have to be in this PR (could be in a follow-up).

Over in reduxjs/redux#3920 , we're updating the Redux core to extract error messages and replace them with error codes in production builds, same as how React does it. That uses a semi-standalone Babel-powered script.

How feasible would it be to apply the same changes to RTK once we're using ESBuild?

Although, having said that... I don't think we even have much in the way of error messages in RTK itself that currently end up in the prod build. There's "invalid reducer" in configureStore, but about the only other error messages I see are in the dev check middleware and those get excluded in prod builds anyway. So, this may be an irrelevant train of thought.

@hardfist
Copy link
Contributor Author

hardfist commented Mar 31, 2021

It's easy just like before, all we need to do is to use babel with the previous plugin to transform the bundled result, but I don't like this babel trick actually , I would prefer follow what typescript handle error message, and it's good for search engine

@hardfist
Copy link
Contributor Author

I think we may need some sourcemap test to verify the sourcemap like esbuild , since the transform is kind of complex here, there may be something wrong with production sourcemappings here

@markerikson
Copy link
Collaborator

markerikson commented Mar 31, 2021

Yeah, not sure how you'd test that, but seems like a good idea. Anything we can do to better verify the contents of the build artifacts is a good thing.

@markerikson markerikson linked an issue Apr 1, 2021 that may be closed by this pull request
# Conflicts:
#	src/entities/sorted_state_adapter.test.ts
@markerikson
Copy link
Collaborator

Awright. At this point we have:

  • All of our original artifacts, approximately the same size
  • The additional "modern" artifacts
  • Sourcemaps
  • Linting + formatting + tests
  • Examples of generating subpackages
  • Dropping TS < 3.8
  • Updated GHA+CSB to Node 14

I've installed the CSB-built package locally into a CRA app and it runs, and I see the latest built .esm file in the DevTools source debugger.

Got at least one thumbs-up on Twitter that the CSB build runs.

I think this PR is at least good to merge in, and we can do any follow-up work in additional PRs if needed.

I'm going to go ahead and merge this.

@hardfist , THANK YOU for your hard work putting this together!

@markerikson markerikson merged commit cb0535d into reduxjs:master Apr 1, 2021
minify: false,
env: '',
},
// ESM, embedded `process`, ES2017 syntax: modern Webpack dev
Copy link

Choose a reason for hiding this comment

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

How a webpack user can use this bundle?

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.

Rework RTK build tooling for 1.6 release The cjs bundle is not fully es5 compatible
4 participants