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

Object form of `browser` in `package.json` not supported #6890

Closed
hitchcott opened this Issue Apr 24, 2016 · 28 comments

Comments

@hitchcott

hitchcott commented Apr 24, 2016

_1 Upvote_ Meteor 1.3.2.4

There appears to be some broken behaviour when installing npm packages that use a browser field in package.json to specify browserify-compatible code.

package.json in question:

  "dependencies": {
    "browserify-sha3": "^0.0.1",
    "sha3": "^1.1.0"
  },
  "browser": {
    "sha3": "browserify-sha3"
  }

Report copied from: axic/keccakjs#1:

Seems like Meteor 1.3 ES6 import is having issues with this package, and is affecting other packages depending on keccakjs.

Reproduction:

meteor create keccak-test
cd keccak-test
meteor npm install
meteor npm install --save keccakjs

client/main.js

import keccakjs from 'keccakjs'

(same outcome with require)


Errors:
node console

Unable to resolve some modules:

  "build/Release/sha3" in /Users/chris/code/test/keccakjs-meteor-test/node_modules/keccakjs/node_modules/sha3/package.json (web.browser)

If you notice problems related to these missing modules, consider running:
  meteor npm install --save build
// running the above has no effect

browser console

Uncaught Error: Cannot find module 'sha3'
require @ install.js:78
meteorInstall.node_modules.keccakjs.index.js @ index.js:1
fileEvaluate @ install.js:141
require @ install.js:75
meteorInstall.client.main.js @ main.js:1
fileEvaluate @ install.js:141
require @ install.js:75
(anonymous function) @ app.js?hash=269ed68…:47

Related:


Seems like it's not respecting the browser field in package.json. Hard-coding the browserified package 'fixes' the problem.

module.exports = require('browserify-sha3').SHA3Hash
@benjamn

This comment has been minimized.

Member

benjamn commented Apr 24, 2016

We do respect the "browser" field, but only when its value is a single string rather than an object containing multiple mappings, since the string case is much more common. See here for the relevant code.

Another very common use of the "browser" field is to provide stubs for Node's built-in modules like fs and http. We provide those stubs if you npm install meteor-node-stubs, which helps reduce the need for object-valued "browser" fields.

I'm open to implementing full support for the "browser" field, though I will also mention two other possible solutions:

  • If the sha3 package specified its own browser-appropriate main module via the "browser" field of its package.json file, then keccakjs wouldn't have to make that decision on behalf of sha3.
  • If keccakjs called require("browserify-sha3") in its client code rather than expecting require("sha3") to work as-is on the client, then it wouldn't have to rely on a magical property of package.json to disambiguate the two, and it would be clearer that those modules are actually not the same.

Supporting non-standard properties of package.json files could be a good idea if it solves a common problem for many people, though it can also lead down a slippery slope, since there are no restrictions on properties allowed in package.json, and it's up to your tools to decide what a custom property means.

@hitchcott hitchcott referenced this issue Apr 24, 2016

Closed

Meteor Support #1

@hitchcott

This comment has been minimized.

hitchcott commented Apr 25, 2016

Keccakjs maintainer has modified to work with Meteor. Thanks for the help both!

@feross

This comment has been minimized.

feross commented May 16, 2016

WebTorrent uses the object version of the "browser" field, so this is broken in Meteor right now.

See: https://github.com/feross/webtorrent/blob/master/package.json#L10-L19

@FinnFrotscher

This comment has been minimized.

FinnFrotscher commented May 16, 2016

+1

@abernix abernix changed the title from NPM Package Import Error: `browser` field in `package.json` not respected? to Object form of `browser` in `package.json` not supported May 17, 2016

@abernix abernix added the bug label May 17, 2016

@vvo vvo referenced this issue May 25, 2016

Closed

Usage in meteor #1024

@vvo

This comment has been minimized.

vvo commented May 25, 2016

Would be nice yes to implement the full browser field like webpack and browserify does: https://github.com/defunctzombie/package-browser-field-spec

@rosscreighton

This comment has been minimized.

rosscreighton commented Aug 13, 2016

jsdom also uses the object version of the "browser" field

@sakulstra

This comment has been minimized.

Contributor

sakulstra commented Oct 24, 2016

@Mediref

This comment has been minimized.

Mediref commented Jan 27, 2017

+1 to get this fixed :)

@ilya-pirogov

This comment has been minimized.

ilya-pirogov commented Mar 2, 2017

@damonmaria

This comment has been minimized.

damonmaria commented Mar 19, 2017

@ilya-pirogov I managed to get this working by first: import 'aws-sdk/lib/browser_loader'

Then in my case working with S3 wasn't a problem when imported this way: import S3 from 'aws-sdk/clients/s3'

If you need to access the global AWS object then import it as: import AWS from 'aws-sdk/browser'

I'm not sure this will work in all circumstances, but it worked for me so far.

@ilya-pirogov

This comment has been minimized.

ilya-pirogov commented Mar 19, 2017

@damonmaria Thank you! It really works. Tested with DynamoDB and SQS.

@javoire

This comment has been minimized.

javoire commented May 23, 2017

Another one here :) https://github.com/mapbox/mapbox-gl-js/blob/master/package.json#L99, meteor bundles the file meant for node, and not the one meant for the browser, which breaks my app. Cheers!

@payner35

This comment has been minimized.

payner35 commented Jun 23, 2017

@pyry

This comment has been minimized.

pyry commented Jul 11, 2017

This issue is present as well with the popular React-Intl package https://github.com/yahoo/react-intl/blob/master/package.json. Correct usage in client is dynamically importing required locales. Actually this https://blog.meteor.com/putting-your-app-on-a-diet-with-meteor-1-5s-bundle-visualizer-6845b685a119 blog post in regards of 1.5 bundle-visualizer and resulting app diet is originally caused by this issue. So hopefully we could get this somehow solved.

@HemalR

This comment has been minimized.

HemalR commented Sep 21, 2017

+1 for a fix. Affecting a lot of useful services:

  1. Algolia Search
  2. Mailguns client side email validation
  3. Keen.io's client side data vis libraries

Does anyone have a work-around I can use in the meantime?

@vangorra

This comment has been minimized.

vangorra commented Sep 21, 2017

For portability, we are using graphql for backed communication and avoided using meteor specific calls. The best work around we found was to ditch meteor all together. It made things much easier in the long run and allowed us to use serverless architecture.
To transition, we built our own client code and used meteor for the backed only. After a bit of that, porting to lambda was pretty straight forward.

@HemalR

This comment has been minimized.

HemalR commented Sep 21, 2017

@aldeed

This comment has been minimized.

Contributor

aldeed commented Oct 26, 2017

I ran into this trying out @firebase/firestore pkg. Here are repro instructions if it helps:

  1. Install Meteor: curl https://install.meteor.com/ | sh
  2. meteor create --bare firestore-issue
  3. cd firestore-issue
  4. meteor npm i --save @firebase/app @firebase/firestore
  5. mkdir client
  6. echo $'import firebase from "@firebase/app";\nimport "@firebase/firestore";' > ./client/main.js
  7. meteor

@benjamn I can look into solving this if you give me a clue of where to start

EDIT: Seems like maybe as simple as updating this line? https://github.com/benjamn/install/blob/master/install.js#L500

@benjamn

This comment has been minimized.

Member

benjamn commented Oct 26, 2017

As with many module resolution features, implementing this functionality requires cooperation between static bundling logic (the ImportScanner and Resolver) and the runtime module system (the npm install package). It's not enough to change how module resolution happens at runtime, because the replacement modules have to be included in the bundle, so the bundler has to obey the same logic.

This functionality would be much simpler to implement if the browser object could only override local modules using relative paths, because it's very easy to establish an alias from one module identifier to another module in the bundle, regardless of where the modules are imported. I call this the "relative style" of browser overrides. For example, this aliasing technique is enough to make the tus-js-client work perfectly, since all of its browser overrides are relative. And because the original module is essentially replaced by the alternate module, there is no runtime overhead to this technique.

Unfortunately, the browser object can also override the resolution of packages, but only when those packages are imported by modules in the same package that contains the package.json with the browser field. I call this the "package style" of browser overrides. This is a totally different situation, because we're not simply replacing the package in question with some other module. The overridden package could still be imported by modules outside the package containing the package.json file, and the real package should be returned in those cases. Since the behavior of package resolution depends on the parent module where the require or import appears, every runtime package resolution has to begin with checking to see if there's a package.json file in the current package that has an object-valued browser field that happens to override the given package that is about to be imported. Needless to say, this adds runtime overhead to every single package import, which is especially unfortunate since this feature is so rarely used.

How does this feature work in Webpack and Browserify? Those bundlers statically rewrite all imported module identifiers as numbers, which means require(id) with a dynamic id string is not allowed, because id can't be statically analyzed and rewritten as a number. That's a convenient simplification for a number of reasons, but it means neither of these bundlers fully implements the so-called "spec" for the browser field, because the browser override doesn't apply to dynamic module identifiers, because dynamic module identifiers are not supported in the first place. It's easy to avoid implementing something slowly if you don't support the slow use case at all!

Meteor's module system guarantees any require(id) import will work at runtime, provided the required module is in the bundle. Since Meteor doesn't rewrite your module identifiers as numbers, we don't have the luxury of hacking package overrides into the static bundling process. The override logic has to be available at runtime, so dynamic module identifiers can be handled properly.

Long story short, I think the relative style of browser overrides is totally worth supporting, but the package style simply isn't worth the additional runtime cost and implementation complexity. I realize it may feel weird to implement this functionality halfway, but the relative style appears to be much more common, and it looks like the relative style would be enough to fix most of the examples that have been cited in this issue.

How do folks feel about that? Is it worth supporting the relative style if we don't support the package style? Is that better than the status quo, at least?

@ljharb

This comment has been minimized.

ljharb commented Oct 26, 2017

Supporting 80% of the browser field is better than 50%, surely (percentages selected are arbitrary, obviously)

@damonmaria

This comment has been minimized.

damonmaria commented Oct 26, 2017

As long as it covers more bases than it does now it has to be an improvement. I've struck this issue in Meteor a couple of times (AWS and Algolia packages) and both only use the "relative style".

@aldeed

This comment has been minimized.

Contributor

aldeed commented Oct 26, 2017

@benjamn Thanks as usual for helping us understand the complexities. I agree that supporting the relative style is the primary request here. It seems to be far more common. Would the complexities preclude supporting the false option for ignoring a module, too? That seems like it would be more common than module overrides. But either way, 👍 for supporting relative style.

If it helps, this is as far as I got looking at browserify implementation before I got pulled into other tasks. :) https://github.com/defunctzombie/node-browser-resolve/blob/master/index.js#L153-L176

@benjamn

This comment has been minimized.

Member

benjamn commented Oct 27, 2017

The false style is easy as long as the key is relative!

@benjamn benjamn added this to the Release 1.6.1 milestone Oct 31, 2017

benjamn added a commit that referenced this issue Nov 6, 2017

@benjamn benjamn self-assigned this Nov 6, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Nov 7, 2017

PR #9311 addresses this issue (by supporting the "relative style" of browser replacements), and has been merged. This should be coming in 1.6.1. Thanks all!

@luchux

This comment has been minimized.

luchux commented Mar 27, 2018

@benjamn I am also having still the error that KayleePop mentioned. Any insights? Seems the object field is still not supported? Any thoughts?

@benjamn

This comment has been minimized.

Member

benjamn commented Mar 27, 2018

@luchux My insights are richly detailed in the comments above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment