Closed romeovs closed 6 years ago
Merging #60 into master will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #60 +/- ##
=======================================
Coverage 92.71% 92.71%
=======================================
Files 2 2
Lines 247 247
Branches 31 31
=======================================
Hits 229 229
Misses 18 18
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 844e4b0...706b003. Read the comment docs.
Hey @romeovs, thanks for the contribution! Will do some testing on this, (specifically around tree-shaking) and report back.
After some digging, here's what I've found:
The root cause of #54 was that I had broken a convention -> module
should refer to a file that uses ES6 modules but crucially, no other ES6 features, to prevent that exact issue. I've made this change, and now you should be able to use this library in non-ES6 compatible targets without a transpile step.
Accepting this PR will break webpack's ability to do tree-shaking, which is not great.
With the current setup, webpack was bundling the browser fallback for node's crypto
library, even though ulid
itself was not using this library in the browser. Instead of forcing users to exclude crypto
with IgnorePlugins
in their webpack configs, I've implemented some fixes, and tested it out. The time required to build drops by 10x, from 1530ms to 148ms! This should help alleviate long build-times further.
Thus, I think the fundamental issues that are raised in this PR have been solved, and this PR is no longer needed. Do you agree?
Totally agree! Closing.
This is related to #54.
When building projects using webpack it is common to exclude the
node_modules
folder from transpiration because this causes the build times to sky rocket (especially in projects with big dependency trees).It should be safe to assume that a package was transpiled so it runs on ES5, to cut down on transpilation times.
The
ulid
package does seem to have a transpiled UMD bundle inlib/index.umd.js
, which webpack supports. The error we saw in #54 (and which I'm seeing in my projects) is due to webpack not picking up on this transpiled bundle, because by default it tries the module alias fields in this order:browser
module
main
The
package.json
ofulid
contains the following relevant fields:So,
module
is being picked up beforemain
by webpack, causing builds to break. As you've said, adding transpilation (and enabling it fornode_modules
) should fix the problem. I propose however, that you add abrowser
field, so by default webpack builds will work as well:I've tested it locally and my webpack build works when these changes are applied.