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

Add support to node-webkit target. #34

Merged
merged 5 commits into from
Jan 28, 2018
Merged

Add support to node-webkit target. #34

merged 5 commits into from
Jan 28, 2018

Conversation

saboya
Copy link
Contributor

@saboya saboya commented Jan 24, 2018

This PR adds support to node-webkit target, necessary to build native modules when using NW.js, an Electron alternative.

Tests are included.

NW.js apparently does not follow a specific versioning pattern to switch ABI, so the getNextTarget function can't be implemented correctly for node-webkit, so I left it untouched.

@ralphtheninja
Copy link
Contributor

LGTM!

@lgeiger
Copy link
Contributor

lgeiger commented Jan 26, 2018

@saboya Thanks for adding this 👍

@ralphtheninja wouldn't this be a breaking change for prebuild since this PR modifies allTargets which is used to determine the default ABIs to prebuild against?

Is this behavior change in behavior desired? If so we should do a new major release, else I would recommend adding node-webkit to something like additionalTargets so we don't break earlier versions of prebuild.

@ralphtheninja
Copy link
Contributor

ralphtheninja commented Jan 26, 2018

Is this behavior change in behavior desired? If so we should do a new major release, else I would recommend adding node-webkit to something like additionalTargets so we don't break earlier versions of prebuild.

Moving to additionalTargets is probably a good idea.

@ralphtheninja
Copy link
Contributor

@lgeiger Maybe should have a discussion on what is ok for default behavior of prebuild. I'll open an issue.

@ralphtheninja
Copy link
Contributor

ralphtheninja commented Jan 28, 2018

@lgeiger I've made PR to adjust this. Hold off with merge.

@ralphtheninja
Copy link
Contributor

Some research below:

screenshot from 2018-01-28 16-09-42

screenshot from 2018-01-28 16-10-20

move node-webkit to additionalTargets + export additionalTargets
@lgeiger lgeiger merged commit 1e324fb into electron:master Jan 28, 2018
@lgeiger
Copy link
Contributor

lgeiger commented Jan 28, 2018

@saboya @ralphtheninja Thanks for your work!

This is released under 2.2.0

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.

3 participants