-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Deploy preview for redux-starter-kit-docs ready! Built with commit 7c6f46d https://deploy-preview-957--redux-starter-kit-docs.netlify.app |
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:
|
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. |
6adf5f1
to
838e322
Compare
done!, I just need to convert bundled esm to umd by rollup |
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 |
838e322
to
c4daceb
Compare
only form umd production ? what about esm&cjs production? |
Uh... just UMD, I think? :) It's That file's gone through a few different changes over time - you can see the changes in the git history. |
e671753
to
8baad0f
Compare
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to handle sourcemap
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 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 Also: it looks like the use of
|
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 check the code length to ensure works,and check the require result of umd bundle |
i normally using ts transpilemodule to transform esbuild bundle result to target es5,if you prefer i can change this |
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: 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 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. |
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 |
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. |
8baad0f
to
bc1e816
Compare
add index.js entry points, down-level to es5, remove extra .js files |
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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo sourcemap merge
The ultimate plan here is to have three entry points:
You can see some of that config here: and here's a CI build from rtk-incubator/rtk-query#177 so you can see the rough build output: 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 |
bc1e816
to
ee0fc59
Compare
@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? |
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. |
yes, exports definitely will break code under nodejs, but I'm little confused, is it really necesary to split file to |
So in theory, bundlers should be picking up the CJS is a fallback for Node. The split-file convention is something React has been using for a while, so we have too. The However, per #652 , files with |
@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
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! |
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 |
I will check it , I just get up |
I DEMAND THAT YOU FIX THIS NOW, BEFORE BREAKFAST!!! :) (seriously, again, appreciate all your efforts on this!) |
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. |
I'm fine with changing whatever type import/export declarations we need to make this work - please go ahead! |
woops, it seems that api-extractor doesn't supports type export yet |
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. |
a25cc46
to
7dbef9f
Compare
after I upgrade api-extractor to latest version, it works now |
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 ? |
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. |
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 |
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 |
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 |
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. |
# Conflicts: # src/entities/sorted_state_adapter.test.ts
Awright. At this point we have:
I've installed the CSB-built package locally into a CRA app and it runs, and I see the latest built 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! |
minify: false, | ||
env: '', | ||
}, | ||
// ESM, embedded `process`, ES2017 syntax: modern Webpack dev |
There was a problem hiding this comment.
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?
just try what esbuild cann't be done for author libraries