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

nwjs require #1161

Closed
guybedford opened this issue Mar 15, 2016 · 5 comments
Closed

nwjs require #1161

guybedford opened this issue Mar 15, 2016 · 5 comments

Comments

@guybedford
Copy link
Member

Since we changed to no longer altering the require global during execution, this breaks Node webkit support as they use a global.require there.

We should always scope require to be undefined for SystemJS module execution, with synchronous reversion of the global value for AMD and global formats where we need to trigger the right UMD pattern.

Then in terms of consistency, it's annoying that we have different rules for each format like this one, but it does seem best as that is the nature of handling compatibility cases.

@guybedford guybedford added the bug label Mar 15, 2016
@MikeKovarik
Copy link

I would very much like to see this fixed as soon as possible since monkey patching my dependencies like this is driving me crazy

var isNWJS = typeof nw == 'object' && typeof nw.App == 'object';
if (isNWJS) {
    module.exports = global.require('net');
} else {...}

@MikeKovarik
Copy link

So I looked into it a bit and found a solution or rather temporal workaround since complexity of SystemJS goes way beyond my understanding. #1168

@guybedford
Copy link
Member Author

Unfortunately the @node namesapce is reserved only for Node core modules itself. I'd suggest something like the following here rather -

System.set('@nwjs/module-x', System.newModule({ default: require('nwjs-module-x') });
global.require = undefined;

Ideally the global require should not be set as that will interfere with global module evaluation.

There is another change that may fix the global require issue here and that would be to move back to using a vm implementation that does encapsulated globals in Node.

Perhaps that is the best fix really, I've created #1183 to track.

@guybedford
Copy link
Member Author

I've added clearing the global scoped require in 8ee8916, pending release.

@guybedford
Copy link
Member Author

Released in 0.19.25.

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

No branches or pull requests

2 participants