-
Notifications
You must be signed in to change notification settings - Fork 7
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
Breaks any other libraries that rely on window #17
Comments
please explain a bit more? |
this library "patches" cometd by adding There might be another library (@salesforce/core or jsforce for example), that checks After patching, this library will think it's operating in a browser, and likely break. I know this because that's exactly what happened when I required this package before running this code from const connection: Connection = await Connection.create({
authInfo: await AuthInfo.create({ username: '[email protected]'})
});
let org = await Org.create({connection});
org.refreshAuth(); |
I guess you could say jsforce is equally to blame here. https://github.com/jsforce/jsforce/blob/master/lib/transport.js#L29 |
fixes >TypeError: Cannot read property 'host' of undefined`. if somehow `window` gets created on the global scope. For example: See cometd/cometd-nodejs-client#17
@ChuckJonas so what suggestions do you have to avoid this?
I can pollute the Alternatively I can pollute the |
I think that makes... Although I'm wondering if you're going to change the Seems like You could do the same with |
I need some adapter code anyways for 2 reasons:
Especially for the latter, again I can pollute the When supporting Makes sense? |
I believe you could do something like this: if(isNode){
require('XMLHttpRequest');
} Alternatively, tools like browserfy and webpack allow you to But I understand the hesitation to muddy the core implementation. Seems like the solution you've outline above should work... Thanks for being responsive! |
@ChuckJonas yeah, there are users that don't use CommonJS but AMD, others that just rely on vanilla JavaScript 4, etc. so I can't really muddy the core implementation. I'm experimenting with my outlined solution above and seems to be working. |
Turns out this fix breaks other libraries. |
as noted in #18 |
Signed-off-by: Simone Bordet <[email protected]>
Pretty much any other library that does a
if(window)
check will no longer operate as expected if you install this.The text was updated successfully, but these errors were encountered: