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

Handle optional, environment-dependent native dependencies #15

Closed
ClementValot opened this issue Aug 3, 2022 · 9 comments
Closed

Handle optional, environment-dependent native dependencies #15

ClementValot opened this issue Aug 3, 2022 · 9 comments

Comments

@ClementValot
Copy link

ClementValot commented Aug 3, 2022

Thought process explained in greater detail here mscdex/ssh2#1201

Some of the native dependencies bundled by rollup-plugin-natives are optional, e.g. in the above example, when installing the ssh2 module, the files sshcrypto.node and cpu-features.node may or may not be present depending on the environment installing the module (In that example, Windows has both, MacOS only has sshcrypto, and Unix has none, from what I could observe), and the ssh2 module defaults to another behavior if the file cannot be found or read.

But when bundling, rollup-plugin-natives writes an explicit require var sshcrypto = require('sshcrypto.node') in the bundled js file, which fails at runtime if the file is not present.

Workaround would be to wrap the require in a try/catch
e.g. var sshcrypto;try{sshcrypto=require("./sshcrypto.node")}catch(err){};

@danielgindi
Copy link
Owner

Is it originally in try-catch clause?

@ClementValot
Copy link
Author

ClementValot commented Aug 4, 2022

Original code in ssh2

try {
binding = require('./crypto/build/Release/sshcrypto.node');
({ AESGCMCipher, ChaChaPolyCipher, GenericCipher,
AESGCMDecipher, ChaChaPolyDecipher, GenericDecipher } = binding);
} catch {}

Code in the bundled file

var sshcrypto=require("./sshcrypto.node");

var sshcrypto$1 = /#PURE/Object.freeze({
proto: null,
'default': sshcrypto
});

var require$$2 = /@PURE/getAugmentedNamespace(sshcrypto$1);

And further in the code, in method requireCrypto() so not top-level

try {
binding = require$$2;
({ AESGCMCipher, ChaChaPolyCipher, GenericCipher,
AESGCMDecipher, ChaChaPolyDecipher, GenericDecipher } = binding);
} catch {}

@danielgindi
Copy link
Owner

So it looks like we're still doing this conditionally aren't we?

@danielgindi
Copy link
Owner

Btw you may need to use ignoreTryCatch feature in commonjs plugin.

@ClementValot
Copy link
Author

Oopsie, I copy-pasted the code that's generated after I applied the band-aid (which is wrapping in a silent try/catch)

I edited the previous message and the issue stays: A conditional require (wrapped in a try/catch, in a method) has been transformed into a top-level require which fails at runtime.

I haven't had the time to tinker with ignoreTryCatch yet, I'll have a read about that :)

@danielgindi
Copy link
Owner

I think that that's your solution. As not all modules are optional within a try-catch, it's not something that the current module should generate. It's handled by commonjs plugin.

@ClementValot
Copy link
Author

The more I play with rollup and the less I understand this issue :(
Whatever the order in which I put the commonsJs, nodeResolve and nativePlugin plugins, and the boolean/'remove' in ignoreTryCatch, the top-level var sshcrypto=require(./sshcrypto) is generated in the bundle.

Does it become an issue with the commonJs plugin which doesn't, for some reason, detect that this specific require should be removed with ignoreTryCatch=true?

@danielgindi
Copy link
Owner

You don't want this require to be removed by commonjs, you want it to stay and inside try-catch :)

@ClementValot
Copy link
Author

Confusion around ignoreTryCatch was solved by a fix in the docs

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