Use process.emitWarning for deprecation warnings

[eritbh]

Already discussed a fair bit on Discord starting here. I can own development of this feature. Summarizing prior conversation:

• It's probably a good idea to use process.emitWarning to emit deprecation warnings, because process warnings are automatically exposed to the user by default and can be controlled through Node CLI flags (including flags specifically for deprecation warnings). This is common in other libraries as well.

• To avoid breaking changes, we can also continue to emit deprecation warnings to the Client warn event, though ironically there's no good way to mark this behavior as deprecated since the warn event is also used for Discord warnings, and we have no way to tell which warnings a user is relying on. However, because process warnings log to the console by default, it's likely that anyone who was relying on deprecation warnings coming from the warn event will notice the extra console output and realize they should update their handling, so it's probably fine to remove deprecation warnings from the warn event a few breaking changes after introducing process deprecation warnings.

  • The migration path for people currently handling/suppressing deprecation warnings via the warn event will probably involve using the --no-deprecation CLI flag to suppress the default console output and/or handling the process warning event.

• Node.js docs say it's a best practice to avoid emitting duplicate warnings to avoid console spam if a deprecated code path is used multiple times. This requires the library to keep track of which deprecation warnings it has already emitted. Given this and the fact that we still need to emit deprecations to the client warn event for now, it probably makes sense to implement this as an internal helper. I've posted the way I handle this in my own library for reference.

  • There is probably not a significant performance impact incurred by doing this in a helper, and the consensus seems to be that maintaining performance doesn't need to be a high priority when deprecating things anyway.

[bsian03]

Do we want to give the user the option to constantly or once for each warn code? Could pop it in client options for example

[DonovanDMC]

This feels like making an option just to make an option, imo it should just be stuck as once, since that's standard practice

[eritbh]

Yeah, the docs are pretty prescriptive about one emit per warning being the right way to go, and i don't think there's any practical reason why you'd want more than one warning intentionally emitted.

[bsian03]

Sticking with emitting once

[bsian03]

Also tbch I'm not that much of a fan of nested functions, are there alternatives we can use? Like pushing warning codes to an array or something. I can benchmark if necessary

[DonovanDMC]

This sounds a lot more readable, and less messy
even gives us custom warning names to play around with

[eritbh]

Using an array/object/Set of already-emitted warning codes is an option, though I'm not sure that would actually eliminate the function-returning-another-function structure of my example - it just changes the if (hasWarned) condition to something else, like if (emittedWarnings.includes(code)).

We can definitely avoid functions returning other functions, it'll just mean that we'll have to inline the boolean checks everywhere in the library a deprecation can be emitted. I did it this way in Yuuko just to avoid repeating the exact same logic across the project, but that project is less focused on raw performance than Eris core - if it has a significant performance impact then I have no issues inlining these checks instead.

Edit: That said, given the warningEmitter function in my example is only called a constant number of times (once per warning code) when the library is first imported, I don't think there's the potential for its performance to diverge significantly.

[bsian03]

Should we have a list of deprecation codes and their corresponding descriptions (thus just passing the code in the deprecation call)? Or just have it typed out in each deprecation call?

[DonovanDMC]

they could just go in constants

Something like:

// Constants.js
const ErrorCodes = {
  SOME_NAME_HERE: "Some description stuff blah blah blah"
}

or even their own file, so we can just directly export them

// ErrorCodes.js
module.exports.SOME_NAME_HERE = "Some description stuff blah blah blah"

[eritbh]

We could do it this way, but at that point what's the difference between this and the method from yuuko? I can't think of any reason a user would need that info programmatically (or any other library that provides that), and looking up the description from the code at emit time rather than binding both at once doesn't seem better for performance.

[DonovanDMC]

it means we wouldn't need to spread the description across several files for the same deprecations, they would be hardcoded in one place
it's more for internals, not for the end user

[bsian03]

Yeah apologies, this is just for internal management, nothing to do with the bot developer

[eritbh]

I'm fine with it as a matter of internal organization.

Since we do have a new file for the emit helper already in #1276 (lib/util/emitDeprecation.js at adc3353), and that's the only place the error codes are going to be used (I don't think it's worth it to use them for client emits right now), I think we can probably just throw them in an object in there and call it good. Then the helper can just take a code instead of a code and a message.

// emitDeprecation.js
const messages = {
  "eris:WHATEVER": "Client#whatever is deprecated. Use Client#somethingElse instead.",
  // ...
};

module.exports = function emitDeprecation(code) {
  // ...
  process.emitWarning(messages[code], "DeprecationWarning", code);
}

// wherever.js
const emitDeprecation = require('./util/emitDeprecation.js');
emitDeprecation("eris:WHATEVER");

This does seem like the most maintainable way to go about it after thinking about it more. Does that sound good?

[JustCat80]

About the deprecation names/codes whatever, we should probably use specific things to make it harder for codes to overlap in different prs and such

For example: Support sending multiple embeds (#1239) would use either "eris:#1239"(maybe not because multiple different warnings so maybe a suffix of the deprecation number within the pr or something) or "eris:embed" or similar depending on how we do it

[JustCat80]

Ah I see my bad, I see what you mean now, yea that works

[bsian03]

I feel like just a name will suffice, e.g. eris:MULTIPLE_EMBEDS. We can link to the PR in the description. Direct commits by abal which deprecates something will also not have a relevant PR (if he also doesn't create an issue), just fyi

[JustCat80]

Fair enough

[DonovanDMC]

I feel like just a name will suffice, e.g. eris:MULTIPLE_EMBEDS. We can link to the PR in the description. Direct commits by abal which deprecates something will also not have a relevant PR (if he also doesn't create an issue), just fyi

true, that would likely work a lot better, the pr number would be just shoehorned in if it was added, and we can just add a See: https://github.com/abalabahaha/eris/blahblahblah

[eritbh]

We could also use the detail argument of the other form of process.emitWarning for this type of "more info" link - it's essentially an extension of the warning message that's displayed on its own line by default. Though I don't think there's anything wrong with just using a longer warning message either, and it's probably simpler to put everything in the warning in light of #1274 (comment), I don't really want to overcomplicate our internal object too much.

I'm fine with eris:MULTIPLE_EMBEDS-type error codes, will update my PR with this strategy later today if nobody objects.

[JustCat80]

To avoid breaking changes, we can also continue to emit deprecation warnings to the Client warn event

Should we also add the warning emits to new prs? or should we just stop doing that and use only emitDeprecation for new things once its merged?

[DonovanDMC]

We're dropping using the warn event for deprecations at all, the old ones are still being emitted for now to avoid breaking any systems that depend on, or look for the current ones

[JustCat80]

Yea I realized that but was just wondering if for a while we still added it for whatever reason vs just not touching the old for now

[eritbh]

If it were up to me, I'd probably discourage adding more client warn emits for new deprecations from here on out.

Edit: Depending on the order stuff gets merged, I can contribute this change to in-flight PRs as well. I wouldn't worry about it.