util: move flagged deprecation to internal/util #18450
Conversation
Two changes in this commit: 1. Deprecation-related logic is moved from buffer.js to internal/util.js That is the new deprecator() method that generates either a deprecation function or a noop, depending on the deprecation type and runtime flags. 2. Buffer pending deprecation stack trace is fixed, so that internal logic does not appear in there, and the first line of the stack should now point at userland code where the deprecated method was called.
ChALkeR
added
buffer
nodejs-github-bot
added
buffer
| if (process.noDeprecation === true) { | ||
| return () => {}; | ||
| } | ||
| if (options.pending && !pendingDeprecation) { |
There was a problem hiding this comment.
Nit: as this if statement does the same as the one above I would just combine these two.
| } | ||
|
|
||
| // code should be set | ||
| if (typeof code !== 'string' || code === '') |
There was a problem hiding this comment.
I personally do not feel that the code === '' check is really necessary but it would actually be nice to check for the right pattern as in: !/^DEP(\d\d\d\d|00XX|0XXX|XXXX)$/.test(code).
That aside: it is in both cases not a ERR_INVALID_ARG_TYPE. It could be a ERR_INVALID_ARG_VALUE.
There was a problem hiding this comment.
I don’ think it’s needed to validate the pattern.
+1 on ERR_INVALID_ARG_VALUE
| if (warned) return; | ||
| warned = true; | ||
| if (codesWarned[code]) return; | ||
| codesWarned[code] = true; |
There was a problem hiding this comment.
As question: is warned here to prevent a lookup in the default case?
There was a problem hiding this comment.
Yes.
- If this specific function has already been called (which it would be in most cases) —
warnedwill betrue, and this will return asap. - The second check (
codesWarned[code]) is needed to catch the situation where e.g. another function has been created using the same deprecation code (or the warning was emitted throughdeprecatebelow) — we still don't want to display the warning second time in that case.
|
The next step will be to pending-deprecate Btw — @nodejs/tsc — we can pending-deprecate SlowBuffer in a semver-minor 9.x release, can't we? |
| } | ||
|
|
||
| // code should be set | ||
| if (typeof code !== 'string' || code === '') |
There was a problem hiding this comment.
I don’ think it’s needed to validate the pattern.
+1 on ERR_INVALID_ARG_VALUE
|
Labeling as |
|
Ping @ChALkeR |
|
Ping @ChALkeR again |
|
@ChALkeR are you still working on this? |
|
@BridgeAR Yes, I am planning to fix this a bit later. It might have conflicts with other more important PRs, so let's land those first. I will take a look in a day or two. |
|
The deprecation PR landed, so this should be good to rebase. |
| // For internal use only. `code` is a required argument. | ||
| // If --no-deprecation is set, then it is a no-op. | ||
| // { pending: true } are visible only when pendingDeprecation is on | ||
| function deprecator(msg, code, options = {}) { |
There was a problem hiding this comment.
nit: can we call this deprecate instead of deprecator
|
Ping @ChALkeR |
|
@BridgeAR Yes, thanks. This is still on my radar, among with several other things… |
|
I checked this again — not usable for the two places where |
Two changes in this commit:
Deprecation-related logic is moved from buffer.js to internal/util.js
That is the new deprecator() method that generates either a
deprecation function or a noop, depending on the deprecation type and
runtime flags.
Buffer pending deprecation stack trace is fixed, so that internal
logic does not appear in there, and the first line of the stack
should now point at userland code where the deprecated method was
called.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
buffer, util
Perhaps I should benchmark this change, I am not exaclty sure if passing
Bufferas an argument in that place is a great idea. This way it's more flexible, but making it an argument ofdeprecatorcould be faster (or not) — I probably need to check that.