Skip to content

Add browser field to package.json#60

Closed
romeovs wants to merge 3 commits intoulid:masterfrom
romeovs:patch-2
Closed

Add browser field to package.json#60
romeovs wants to merge 3 commits intoulid:masterfrom
romeovs:patch-2

Conversation

@romeovs
Copy link

@romeovs romeovs commented Jan 2, 2018

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 in lib/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:

  1. browser
  2. module
  3. main

The package.json of ulid contains the following relevant fields:

  "main": "./lib/index.umd.js",
  "module": "./lib/index.js",

So, module is being picked up before main by webpack, causing builds to break.
As you've said, adding transpilation (and enabling it for node_modules) should fix the problem. I propose however, that you add a browser field, so by default webpack builds will work as well:

  "main": "./lib/index.umd.js",
+ "browser": "./lib/index.umd.js",
  "module": "./lib/index.js",

I've tested it locally and my webpack build works when these changes are applied.

@codecov-io
Copy link

codecov-io commented Jan 2, 2018

Codecov Report

Merging #60 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

@alizain
Copy link
Collaborator

alizain commented Jan 2, 2018

Hey @romeovs, thanks for the contribution! Will do some testing on this, (specifically around tree-shaking) and report back.

@alizain
Copy link
Collaborator

alizain commented Jan 3, 2018

After some digging, here's what I've found:

  • The root cause of error when bundle for producation #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?

@romeovs
Copy link
Author

romeovs commented Jan 3, 2018

Totally agree! Closing.

@romeovs romeovs closed this Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants