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

win: enable delay-load hook by default #708

Closed

Conversation

Fishrock123
Copy link
Contributor

Enables the following commit by default:
52ceec3

See also:
nodejs/node@3bda6cb

Seeing if we can get this in, even into a major, to land in npm, so that we don't need to keep patching it in io.js/node.

I talked with @othiym23 a bit last Friday and he seemed to think it wasn't major-worthy. In io.js we wated to do it in a major.

@TooTallNate what are your thoughts on this?

cc @nodejs/tsc?

Enables the following commit by default:
52ceec3

See also:
nodejs/node@3bda6cb
@rvagg
Copy link
Member

rvagg commented Aug 31, 2015

this is one for @nodejs/platform-windows, specifically @piscisaureus who introduced it. I think this is more for io.js than node so maybe not as urgent?

@Fishrock123
Copy link
Contributor Author

I think this is more for io.js than node so maybe not as urgent?

@othiym23 seemed to think it also applied to electron and nw.js. If it needs to be in a major I'd prefer to get it into npm@3.

@rvagg
Copy link
Member

rvagg commented Sep 4, 2015

@Fishrock123 let's merge this and float on v4

@rvagg
Copy link
Member

rvagg commented Sep 5, 2015

so, I know we're in a transition phase here and still wanting to defer to @TooTallNate somewhat but I'm also assuming that he's happy for us to take the incentive to get stuff done, so I've gone ahead and merged this and used the core style of merging including adding metadata, I've also added a semver-major here (so we could use changelog-maker here if we want to as well). It's hard for me to assess whether this really is strictly a semver-major but given that binaries compiled with this change will operate differently on Windows I think we can afford to be cautious and make this bump to v3.0.0.

@rvagg rvagg closed this Sep 5, 2015
Fishrock123 added a commit that referenced this pull request Sep 5, 2015
Enables the following commit by default:
52ceec3

See also:
nodejs/node@3bda6cb

PR-URL: #708
Reviewed-By: Rod Vagg <rod@vagg.org>
@TooTallNate
Copy link
Contributor

👍 Feel free to push to master. I've also added @rvagg and @Fishrock123 as owners on the npm package. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants