Skip to content
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
IstoraMandiri opened this issue Apr 24, 2016 · 29 comments
Closed

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

IstoraMandiri opened this issue Apr 24, 2016 · 29 comments
Assignees
Labels
confirmed We want to fix or implement it Impact:few in-development We are already working on it Type:Bug Type:Feature
Milestone

Comments

@IstoraMandiri
Copy link

IstoraMandiri 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
Copy link
Contributor

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.

@IstoraMandiri
Copy link
Author

IstoraMandiri commented Apr 25, 2016

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

@feross
Copy link

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

@LazerJesus
Copy link

+1

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

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
Copy link

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

@sakulstra
Copy link
Contributor

Superagent https://github.com/visionmedia/superagent/blob/master/package.json#L53 and spotify-web-api-node https://github.com/thelinmichael/spotify-web-api-node/blob/master/package.json#L50 use the object version of browser field and are broken :/

@Mediref
Copy link

Mediref commented Jan 27, 2017

+1 to get this fixed :)

@ilya-pirogov
Copy link

AWS SDK is broken too. https://github.com/aws/aws-sdk-js/blob/master/package.json#L49-L54

@damonmaria
Copy link

@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
Copy link

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

@javoire
Copy link
Contributor

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!

@HemalR
Copy link

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
Copy link

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
Copy link

HemalR commented Sep 21, 2017 via email

@aldeed
Copy link
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
Copy link
Contributor

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
Copy link

ljharb commented Oct 26, 2017

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

@damonmaria
Copy link

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
Copy link
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
Copy link
Contributor

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 pushed a commit that referenced this issue Nov 6, 2017
@benjamn benjamn self-assigned this Nov 6, 2017
@benjamn benjamn added confirmed We want to fix or implement it Type:Feature Impact:few in-development We are already working on it known-fix labels Nov 6, 2017
@hwillson
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

benjamn commented Mar 27, 2018

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

@houshuang
Copy link

The latest MobX version uses an object-form of browser in package.json, which Meteor cannot deal with: mobxjs/mobx#1844

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed We want to fix or implement it Impact:few in-development We are already working on it Type:Bug Type:Feature
Projects
None yet
Development

No branches or pull requests