Skip to content

Conversation

@dbkr
Copy link
Contributor

@dbkr dbkr commented Aug 2, 2018

The 'browser' field should be a node module intended for use in
a web browser, eg. using the window object or similar as per
https://docs.npmjs.com/files/package.json#browser . It's what
webpack imports if the key exists. This points it to browserified
source code which isn't what webpack wants.

Also the sanitize-html.js published to npm seems to be an empty
module, I don't know why this is yet, but either way I don't
believe this key should be here. It description says it was to make
packers use the transpiled version rather than the ES6 version,
but it was already pointing at dist/index.js which is the babel
transpiled version.

Fixes #241

The 'browser' field should be a node module intended for use in
a web browser, eg. using the `window` object or similar as per
https://docs.npmjs.com/files/package.json#browser . It's what
webpack imports if the key exists. This points it to browserified
source code which isn't what webpack wants.

Also the sanitize-html.js published to npm seems to be an empty
module, I don't know why this is yet, but either way I don't
believe this key should be here.
Copy link

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

browser doesn't mean it has to be bundled, it just means it shouldn't use (non-polyfillable) browser-incompatible code. It should still be unbundled so dependencies can be deduped if another package depends on the same dependencies, so they only end up in the final bundle once.

@boutell boutell merged commit e290185 into apostrophecms:master Aug 2, 2018
@boutell
Copy link
Member

boutell commented Aug 2, 2018

Hmm. This fix gives me back good files in dist/, even if I don't restore the old package-lock.json first.

I published 1.18.4. Please take a look. I'm out today but doing my best to check in. Thanks!

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