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

Improve compatibility with application bundlers #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skozin
Copy link

@skozin skozin commented Mar 25, 2018

This PR allows eccrypto to be used with bundlers that package Node.js app into a single executable (specifically, with nexe/nexe). There are two changes.

Avoid using return outside of a function

Node.js allows out-of-function return due to the way globals are implemented (it wraps all modules into a function), but it's not a correct JavaScript. One of the side-effects of this is that bundlers like nexe don't work with applications that use eccrypto. Bundlers use AST parsers like Acorn to parse all sources into AST and then combine them into a single JS file, and parsers choke on an out-of-function return.

Use .node extension when requiring native module

Bundlers that support bundling native modules, like nexe, need to know which require calls load native modules. To distinguish them from usual require calls, nexe looks at the extension which the require'd path ends with, and assumes it's native module if it ends with .node. Currently, eccrypto requires its native lib, ./build/Release/ecdh, without the extension, which prevents nexe from handling it specially.

Node.js allows this due to the way globals are implemented there (it wraps
all modules into a function), but it's not a correct JavaScript. One of
the side-effects of this is that bundlers like nexe/nexe don't work with
applications that use eccrypto, as these bundlers use Acorn to parse
all sources into AST and then combine them into a single JS file, and
Acorn chokes on out-of-function return.
This allow bundlers like nexe/nexe detect this as a
native module import and process it accordingly.
@skozin
Copy link
Author

skozin commented Mar 25, 2018

Not sure why browser tests fail, it appears to be not connected with the code changes.

@JBaczuk JBaczuk added this to the v1.1.0 milestone Nov 10, 2018
@JBaczuk
Copy link
Contributor

JBaczuk commented Dec 10, 2018

This needs to be rebased from 1.1 and retested, the browser tests have been fixed in the 1.1 branch.

@JBaczuk JBaczuk removed this from the v1.1.0 milestone Dec 10, 2018
@boxfox619
Copy link

i have problem like this error

Module parse failed: 'return' outside of function (25:4)
You may need an appropriate loader to handle this file type.
| console.error(e);
| console.error('Reverting to browser version');
| return (module.exports = require("./browser"));
| }
| }

So, i remove the return statement in node_modules before bundling.
I tried to make a PR but it was already here.
I hope this PR will be reflected soon.

@shrpne
Copy link

shrpne commented Nov 20, 2020

I have the same error.

Can it be rebased and merged, please?

@Vitomir2
Copy link

Lol, why are these fixes left so long? @JBaczuk @skozin

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

Successfully merging this pull request may close these issues.

5 participants