No env specific bundle for ESM#142
Conversation
|
Btw, I've noticed that for |
| opts.name = opts.name || appPackageJson.name; | ||
| opts.input = await getInputs(opts.entry, appPackageJson.source); | ||
| const buildConfigs = createBuildConfigs(opts); | ||
| await util.promisify(mkdirp)(resolveApp('dist')); |
There was a problem hiding this comment.
I haven't fully understood why this line was originally inside the condition. Why it shouldn't be creating dist folder no matter of what format is chosen?
|
Perhaps I should also rename those umd bundles as drafted in #141 (comment)? It would be breaking change unless making alias files for that as well. On the other hand, having |
|
I think UMD story similar to react is ok too. |
|
I would recommend to rename |
|
@TrySound Well, in that case I would have to change the whole Also, React actually has env specific CJS bundles. Perhaps it makes sense there? |
|
Yeah, both umd and cjs are ok. The problem only esm bundle right now. @FredyC I think from tsdx point of view it can support both. |
|
Alright, another version. The CJS is back in both dev/prod builds. It also adds |
83625ad to
97fe66f
Compare
97fe66f to
756250a
Compare
| )}.${format}.${env}.js`, | ||
| file: outputName, | ||
| // Pass through the file format | ||
| format, |
There was a problem hiding this comment.
You may add the same condition here to highlight migration to esm
There was a problem hiding this comment.
Well, I am not sure to call it a migration. The babel plugin expects the "es" only (according to typings). Strangely enough, the rollup expects "esm" in the format option. I wonder how is it possible it actually worked till now. It's a bit of mess.
There was a problem hiding this comment.
Babel do not use es value anywhere. The type is defined in this module.
There was a problem hiding this comment.
Format is being passed to Babel in here, but who knows what's its purpose there https://github.com/palmerhq/tsdx/blob/943648edb06cd131f2c1751120cd1301804b480b/src/createRollupConfig.ts#L163
Either way, I've used the same condition for a rollup format.
There was a problem hiding this comment.
I mean babelOptions is defined here too. Format is only used to check non-commonjs
https://github.com/palmerhq/tsdx/blob/d111c131a5e5cf19d62b98e123060c2ab55c43da/src/createRollupConfig.ts#L16-L50
There was a problem hiding this comment.
Doh! How could I've missed that :) Ok, it's now consolidated as "esm" everywhere with a check for "es" to avoid breakage.
f6a89c5 to
175c544
Compare
|
So...anything else missing here? Would like to get it out soon-ish so we can publish a fixed package. I am kinda surprised that nobody else noticed it yet considering that Webpack grabs the ESM build by default. |
|
Latest release of formik uses old build. I guess that's why no one noticed. |
|
Can you release it this week, please? |
|
Yup. On it meow. |
|
@allcontributors please add @FredyC for code, docs, test, ideas, bug |
|
I've put up a pull request to add @FredyC! 🎉 |
|
@allcontributors please add @TrySound for review, questions, ideas |
|
I've put up a pull request to add @TrySound! 🎉 |
Fixes #140
An alternative (and correct) approach based on a discussion in #141
I've keptindex.jswhich points to a singlename.cjs.jsfile to keep breaking changes at a minimum. It could probably be removed eventually.Besides some slight cleanup, the major change is a single ESM build that isn't env specific. Production bundles also get the
.minin the name.