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

Some NPM packages don't work #406

Closed
bmcmahen opened this issue Mar 28, 2015 · 16 comments
Closed

Some NPM packages don't work #406

bmcmahen opened this issue Mar 28, 2015 · 16 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@bmcmahen
Copy link

I've come across a few npm modules that don't appear to work correctly. Notably, event-emitter has issues importing es5-ext/object/assign, and debug doesn't appear to work with the chrome dev-tools. I suspect that small alterations to the module themselves (to take into account yet another working environment) may be necessary, but there could also be issues with the packager, too.

@novadev94
Copy link

I can't get it work with lodash too.

var _ = require('lodash'); and var lodash = require('lodash'); both return an empty object.

Take a look at the bundle file generated by packager, the code for lodash is there.

@jgable
Copy link
Contributor

jgable commented Mar 28, 2015

I've also had a hard time getting some modules to work. I was able to get lodash to work by using the lodash-node version. But, things like Backbone are not.

It looks like the packager thinks the exported function is a factory for creating the module, but the factory (in lodash) is not actually detecting a node environment and instead the export ends up being undefined.

I'll keep digging in the bundle and report if I find anything useful.

@nicinabox
Copy link
Contributor

I had to use the lodash-cli to build a node version for React: lodash exports=node

@jgable
Copy link
Contributor

jgable commented Mar 28, 2015

It looks like, at least in the case of backbone (and probably regular underscore/lodash), that it checks for a define && define.amd to determine whether to use AMD versus commonJS style exports.

Why does the packager set define.amd if it's preferring commonJS exports? Is it just because that's what the polyfill had initially? Can we remove that?

I guess, the follow up question would be, if we do allow AMD style defines, why aren't they working for lodash, et. al.

@jgable
Copy link
Contributor

jgable commented Mar 29, 2015

@nicinabox The lodash-node package on npm should work for you without making a custom build; at least it has for me so far. npm install lodash-node --save and then var _ = require('lodash-node');.

@nicinabox
Copy link
Contributor

I think it's been deprecated though.

@jgable
Copy link
Contributor

jgable commented Mar 29, 2015

When I comment out this define.amd = {} I am able to load lodash and backbone (well, backbone is a little trickier because it depends on underscore) without a problem.

@vjeux
Copy link
Contributor

vjeux commented Mar 29, 2015

For the amd issue, here's our code responsible for it. https://github.com/facebook/react-native/blob/master/packager/react-packager/src/DependencyResolver/haste/polyfills/require.js#L597 I'm unsure why we define this global.

@vjeux
Copy link
Contributor

vjeux commented Mar 29, 2015

Woops @jgable beat me to it!

@siuying
Copy link

siuying commented Mar 29, 2015

package he shows same issue.

    // Some AMD build optimizers, like r.js, check for specific condition patterns
    // like the following:
    if (
        typeof define == 'function' &&
        typeof define.amd == 'object' &&
        define.amd
    ) {
        define(function() {
            return he;
        });
    }   else if (freeExports && !freeExports.nodeType) {
        if (freeModule) { // in Node.js or RingoJS v0.8.0+
            freeModule.exports = he;
        } else { // in Narwhal or RingoJS v0.7.0-
            for (var key in he) {
                has(he, key) && (freeExports[key] = he[key]);
            }
        }
    } else { // in Rhino or a web browser
        root.he = he;
    }

@OlivierFortin
Copy link

I have the same issue with async,

var async = require('async');
console.log(_);// {}

@amasad
Copy link
Contributor

amasad commented Apr 1, 2015

fixed via #581 will publish v0.3.2 soon

@amasad amasad closed this as completed Apr 1, 2015
vjeux pushed a commit to vjeux/react-native that referenced this issue Apr 13, 2015
Summary:
See facebook#406

Made sure the jest tests pass but didn't know a good unit test to add for this.
Closes facebook#427
Github Author: Jacob Gable <jacob.gable@gmail.com>

Test Plan:
* ./runJestTests
* start app and click around
vjeux pushed a commit to vjeux/react-native that referenced this issue Apr 14, 2015
Summary:
See facebook#406

Made sure the jest tests pass but didn't know a good unit test to add for this.
Closes facebook#427
Github Author: Jacob Gable <jacob.gable@gmail.com>

Test Plan:
* ./runJestTests
* start app and click around
vjeux pushed a commit to vjeux/react-native that referenced this issue Apr 15, 2015
Summary:
See facebook#406

Made sure the jest tests pass but didn't know a good unit test to add for this.
Closes facebook#427
Github Author: Jacob Gable <jacob.gable@gmail.com>

Test Plan:
* ./runJestTests
* start app and click around
@broonage
Copy link

Naive question alert, but why not support AMD?

@vjeux
Copy link
Contributor

vjeux commented Aug 20, 2015

@broonage: we're building a new js environment (others are browser, web worker, node) and need to figure out what we support. We want to support as few things as possible so that it is easier to maintain and has less bugs overall. It turns out that the community pretty much settled on commonjs and not AMD so this is what we chose to implement. Having two things to do the same thing is usually not a great idea.

If in the future AMD makes a big comeback and there are big incentives to implement it, we could. But trying to keep it as simple as possible for now :)

@broonage
Copy link

@vjeux: Thanks, good to know. I'm trying to re-use some existing web modules in a project which use AMD (define), so I have to edit all the files so they are working with require() calls and module.exports=CoolReactNativeModule. I guess no other way about it. :)
Thanks again.

@vjeux
Copy link
Contributor

vjeux commented Aug 20, 2015

@cpojer would suggest you to write a codemod for this :p

@facebook facebook locked as resolved and limited conversation to collaborators May 29, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

10 participants