Revamped build setup - provide module build #49
Conversation
| @@ -9,6 +9,7 @@ | |||
| ], | |||
| "homepage": "http://zpao.github.io/qrcode.react", | |||
| "main": "lib/index.js", | |||
| "module": "es/index.js", | |||
There was a problem hiding this comment.
Default exports in ES modules are a dangerous with Webpack. I'd omit this.
There was a problem hiding this comment.
Why are they dangerous? ESM modules help webpack to make your bundles smaller.
There was a problem hiding this comment.
If you use CommonJS modules and Webpacks default config, any module that exposes both a CJS version as main and an ESM version as module will
- work in NodeJS
- fail when bundled through Webpack
The reason is that Webpack transforms the default export to module.exports.default = QRCode; while rollup turns it into module.exports = QRCode;, making the whole thing a pain for CJS users writing isomorphic code.
See for example this issue: civiccc/react-waypoint#246
There was a problem hiding this comment.
The demo that gets built doesn't work, for this very reason. Please ensure it does. That could be done just by tweaking our webpack config to not look at the module field but that means anybody using this package would need to do the same, so I agree with @realityking on just removing it for now.
|
@zpao Could we get something like this merged? 🙏 |
There was a problem hiding this comment.
Sorry for the long holdup! I have a few comments, but otherwise I don't have any problem with building an ES module. Thanks for taking it on. I'll try to be faster on the followup 😅
#53 landed so you'll need to rebase this. I was doing it locally and ran into issues with yarn.lock not rebasing properly (needed to wipe node_modules and run yarn again to pick up a dep), so just beware.
| @@ -0,0 +1,23 @@ | |||
| const loose = true; | |||
There was a problem hiding this comment.
I'm not really a fan of loose mode given possible divergence from spec behavior. Can you explain why you want to do this?
FWIW, your size wins are pretty much all coming from this.
| modules: false, | ||
| loose, | ||
| targets: { | ||
| browsers: ['>0.25%'] |
| ], | ||
| plugins: [ | ||
| ['@babel/plugin-proposal-class-properties', { loose }], | ||
| ['@babel/plugin-proposal-object-rest-spread', { loose }], |
| plugins: [ | ||
| ['@babel/plugin-proposal-class-properties', { loose }], | ||
| ['@babel/plugin-proposal-object-rest-spread', { loose }], | ||
| ['transform-react-remove-prop-types', { mode: 'unsafe-wrap' }], |
There was a problem hiding this comment.
This is actually going to break things in production. I currently iterate over propTypes in shouldComponentUpdate. You could switch to wrap mode but then sCU won't do anything. I'm not sure why I wrote it like I did though, that should really just be a shallowEqual check…
Lines 91 to 95 in c391441
| @@ -9,6 +9,7 @@ | |||
| ], | |||
| "homepage": "http://zpao.github.io/qrcode.react", | |||
| "main": "lib/index.js", | |||
| "module": "es/index.js", | |||
There was a problem hiding this comment.
The demo that gets built doesn't work, for this very reason. Please ensure it does. That could be done just by tweaking our webpack config to not look at the module field but that means anybody using this package would need to do the same, so I agree with @realityking on just removing it for now.
| @@ -201,7 +198,6 @@ class QRCodeSVG extends React.Component<QRProps> { | |||
| // For level 40, 31329 -> 2 | |||
| const ops = []; | |||
| cells.forEach(function(row, y) { | |||
| let lastIsDark = false; | |||
There was a problem hiding this comment.
This is unrelated. I'll remove that separately.
|
Will be doing this in #174. Sorry I never got to it sooner! |
Size improvements 16% 💰 :
before this PR - 2103 (min+gzipped)
after this PR - 1754 (min+gzipped)