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

refactor: use bindings to load .node file #1263

Closed
wants to merge 1 commit into from

Conversation

codebytere
Copy link

@codebytere codebytere commented Dec 19, 2019

This PR alters .node file loading to use bindings. I'd like to make this change because it enables Webpack to understand how to handle this module, a need which would apply to, for example, Electron apps which both need to use native node modules and use Webpack.

This should not disrupt any existing functionality, and bindings itself only has one dependency, which makes this change have minimal impact as far as added dependencies are concerned.

Thank you!

@codebytere codebytere changed the title chore: use bindings to load .node file refactor: use bindings to load .node file Dec 19, 2019
@kewde
Copy link
Collaborator

kewde commented Feb 1, 2020

Thank you for your contribution @codebytere

I'm closing and re-opening the PR to trigger the CI again because I noticed some false positives.
The failures in the CI seem unrelated to any of these changes.

@kewde kewde closed this Feb 1, 2020
@kewde kewde reopened this Feb 1, 2020
@codebytere
Copy link
Author

@kewde i think they're flakes too - please let me know if there's anything else i need to take care of for this PR otherwise :)

@kewde
Copy link
Collaborator

kewde commented Feb 9, 2020

I've had the time to inspect this a little bit deeper.

The behavior of bindings is to check all the possible locations that a native addon would be built at, and returns the first one that loads successfully.

The behavior of find() in node-pre-gyp builds up the path using information from package.json, the node runtime, etc. In short, find() will guarantee that it tries to load the correct version.

If everything goes well and only the right bindings are downloaded/built then this shouldn't cause any problems. But I've seen many situations where people (myself included) have updated their Node environment (e.g upgraded from Node 12 to Node 13) but the bindings (.node files) of the older version are not cleaned up and still available in the directory. They are not cleaned up properly. I suspect this PR may cause the loading of older bindings.

@codebytere
Copy link
Author

@kewde do you see a path forward here in that event?

@kewde
Copy link
Collaborator

kewde commented Feb 10, 2020

@codebytere I just hope the bindings library fails to load .node bindings that were compiled for an older ABI version. I will test this out, if it fails to load the older bindings then the behavior remains unchanged.

I don't see it as a blocker for this pull request though.

@kewde
Copy link
Collaborator

kewde commented Feb 11, 2020

@codebytere
Does using #1268 also solve the webpack issue?
I'm more leaning to accepting that one simply because:

  1. no additional dependency
  2. same behavior as before

Please let me know if I'm missing something.
Thank you.

@kewde kewde closed this Mar 13, 2020
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.

None yet

2 participants