GeezerVison

See #5504

cc @nodejs/collaborators @nodejs/ctc

CI: https://ci.nodejs.org/job/node-test-pull-request/1806/

Is this semver-major?

It is.

Can you remove BINARY from enum encoding in src/node.h as well?

Oh wait, the intent is to change the default, not remove it altogether? LGTM in that case.

Thank you, @bnoordhuis .

How should we handle this major breakage @nodejs/ctc ? There is hardly a way to deprecate it, because it is a change of default value. Should we just land it and write some blog posts on this change?

Yeah, I think that's likely best. Not sure there's a better way.

LGTM

Landed in b010c87, thank you!

@indutny Please don't land major changes so fast?

Yeah... for stuff like this perhaps give it a day or two to give more people a chance to review. I wouldn't anticipate any issues with this one but still...

Oh, sorry... What's our process on this?

For a major I think it's something like at least 48 hours.

For reference (so there can be broad agreement or else we can talk about updating it to be more accurate) from the current version of the Collaborator Guide:

Before landing pull requests, sufficient time should be left for input from other Collaborators. Leave at least 48 hours during the week and 72 hours over weekends to account for international time differences and work schedules. Trivial changes (e.g. those which fix minor bugs or improve performance without affecting API or causing other wide-reaching impact) may be landed after a shorter delay.

It does not appear to be in the Collaborator Guide, but the doc for onboarding, or at least the one I bookmarked at my onboarding, says:

semver-major (breaking) changes must be reviewed in some form by the CTC

If that's still correct, it's probably worth mentioning in the Collaborator Guide. (We may also want to get at least a tiny bit more specific than "in some form".)

For anything but trivial or otherwise urgent changes we should be leaving at least 48 hours. This is kind of major and deserves the chance for more discussion to flush out any possible objections.

Just saw websockets/ws#716. Did someone write a blog post / example in docs on what Node users need to change (for example, with hash#update calls)?

This PR made sweeping backwards-incompatible changes to the crypto API, but did not include documentation. cf. #10265

@sam-github clearly this PR changes all occurrences of binary to utf8 in the docs. What kind of documentation do you mean?

So, that basically boils down to "existing behaviour was undocumented, so there were no docs to change", which is one way to look at it.

I'm not necessarily saying that this PR shouldn't have landed without you being forced to document the crypto APIs, but I am saying that users of APIs like pbkdf2() (see #10265) could never have figured out from the docs that there has been a change, which is a problem that needs flagging.

Which is why I commented here, so that future readers of this PR can know that the changes it made weren't documented.

I agree we could do better on this. However, careful reader of docs would notice:

https://github.com/nodejs/node/pull/5522/files#diff-efc9444c79e30e68a167275387a55aa6R813

Which says that default encoding is utf8.

So I can't say that it was completely undocumented, just wasn't in pbkdf documentation itself.