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

util: mark deprecated objects with symbol #52568

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

Fixes #22543

This PR marks deprecated objects with a custom symbol, to make it easier for developers to know what functions have been deprecated.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 17, 2024
return deprecated;
}

deprecate.isDeprecated = SymbolFor('nodejs.util.deprecate.isDeprecated');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason this is a global symbol in the registry? Cross version compat if we vendor deprecated stuff e.g. from strearms?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no specific reason. I noticed that other Symbols within the util object were formatted the same way, and I wanted to follow the trend, is there a better way?

@cola119
Copy link
Member

cola119 commented Apr 18, 2024

Can you please add tests?

lib/internal/util.js Outdated Show resolved Hide resolved
@RedYetiDev
Copy link
Member Author

I've added
util.isDeprecated(object) (Function)
util.deprecate.isDeprecated (Symbol)

I'll add some tests later today

Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we ship this, we then have it applied to every function, class constructor etc... we have deprecated, which seems a big change, but I dont see the return. Can you please describe more the use case?

@RedYetiDev
Copy link
Member Author

If we ship this, we then have it applies to every function, class constructor etc... we have deprecated, which seems a big change, but I dont see the return. Can you please describe more the use case?

I believe that the comments on issue #22543 give some use cases, but I see it as a way for developers to anticipate deprecations. For example, they can use the punycode module when available, but otherwise use some other method.

@marco-ippolito
Copy link
Member

marco-ippolito commented Apr 18, 2024

If we ship this, we then have it applies to every function, class constructor etc... we have deprecated, which seems a big change, but I dont see the return. Can you please describe more the use case?

I believe that the comments on issue #22543 give some use cases, but I see it as a way for developers to anticipate deprecations. For example, they can use the punycode module when available, but otherwise use some other method.

punycode is deprecated 😄 but thats beside the point, if we want developers to know we runtime deprecate so they will see the warning.

@RedYetiDev
Copy link
Member Author

I'm aware that puny_code is deprecated, that's why it was my example. The warnings are nice, but they aren't programmatic, an end user can't really use them in their code

@marco-ippolito
Copy link
Member

But how do you know something is deprecated? Manually calling util.isDeprecated for every single api they use if they use are deprecated or am I missing something?

@RedYetiDev
Copy link
Member Author

But how do you know something is deprecated? Manually calling util.isDeprecated for every single api they use if they use are deprecated or am I missing something?

Yes.
Any function deprecated with util.deprecate will have the new symbol. (This includes developer-deprecated functions)

@marco-ippolito
Copy link
Member

marco-ippolito commented Apr 18, 2024

I'm trying to immagine the use case:
If you dont know something is deprecated (you can always read the documentation) every time you use a function you add the check:

const { readFile } = require('node:fs');

if(util.isDeprecated(readFile)){
// do what? 😅
}

You will know with a warning anyways

@RedYetiDev
Copy link
Member Author

True, maybe this PR isn't the best, I can always close it and focus on a different part of Node.

@RedYetiDev
Copy link
Member Author

I just found a use case, I was just trying to print NodeJS internals, but only non-deprecated ones, as their was no way to do it

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: util: isDeprecated()
6 participants