-
Notifications
You must be signed in to change notification settings - Fork 507
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
Create ES dev bundle same as CJS #141
Conversation
I don’t think any bundler is setup to use this build though. Only one file can be noted as the esm entry in pkg.json
…--
Jared
________________________________
From: Daniel K. <notifications@github.com>
Sent: Wednesday, June 12, 2019 2:53 PM
To: palmerhq/tsdx
Cc: Subscribed
Subject: [palmerhq/tsdx] Create ES dev bundle same as CJS (#141)
Fixes #140<#140>
Tested it against a project that has shown the problem and works correctly there.
Btw: It seems that tests setup is wrong, no test file is executed.
________________________________
You can view, comment on, or merge this pull request online at:
#141
Commit Summary
* Create ES dev bundle same as CJS
File Changes
* M README.md<https://github.com/palmerhq/tsdx/pull/141/files#diff-0> (2)
* M src/index.ts<https://github.com/palmerhq/tsdx/pull/141/files#diff-1> (62)
* M test/tests/tsdx-build.test.js<https://github.com/palmerhq/tsdx/pull/141/files#diff-2> (4)
Patch Links:
* https://github.com/palmerhq/tsdx/pull/141.patch
* https://github.com/palmerhq/tsdx/pull/141.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#141>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AA67IG34UUUYULWSMZOSUCTP2FA2HANCNFSM4HXO3Q6A>.
|
@jaredpalmer Well, the assumption is that you will use |
This way you may end up with a lot of duplicated code in your bundle. |
@TrySound Well yea, but how does that matter? Each bundler/environment will pick whatever they need. It's not like you will use all of them at once. UMD is the ultimate copy and would forbid for that format to exists if I could :) |
Ah, finally it's green. Please squash it, I got a bit overboard with commits here 😎 |
The point is to drop commonjs completely. It should not be used to import esm. It will break a lot of stuff. |
@TrySound That's a bad idea. NodeJS is still not fully ESM capable, especially for people stuck at V8 or something. You cannot ask of them to transpile 3rd party modules. |
I'm not gonna ask to transpile. Here's a kind of perfect build I recommend.
|
You cannot conditionally import ESM the way you can conditionally require in CJS like we do with the CJS entry. I confirmed this with @developit. Thus, there is no use case for a ES dev build. |
@jaredpalmer Where is the simplifity of early days rollup config? It's full of unreadable conditions now. And how you want users to build their apps with production mode messages? |
I see what you mean. That's bad... How to solve the original issue then? It breaks the development builds if a production build is used. |
Just remove |
@TrySound I see what you mean. That makes sense and I assume it's the way microbundle does since it does not have such if/else logic in the output. Oh well, should have been thinking more before diving into this PR. I suppose let's close it and do it with a clean slate. |
Btw, I wonder, does it actually work by default that Webpack would run replace on NODE_ENV? It's been quite a while I've configured Webpack by myself and I do remember some awkward DefinePlugin or whatnot. |
@FredyC webpack 4 has |
Fixes #140
Tested it against a project that has shown the problem and works correctly there.
Btw: It seems that tests setup is wrong, no test file is executed.