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

Fully deprecate require('constants') #26012

Closed
ZebraFlesh opened this issue Feb 8, 2019 · 12 comments
Closed

Fully deprecate require('constants') #26012

ZebraFlesh opened this issue Feb 8, 2019 · 12 comments
Labels
deprecations Issues and PRs related to deprecations. feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem.

Comments

@ZebraFlesh
Copy link

  • Version: 10.15.1
  • Platform: Windows 10 64-bit
  • Subsystem: constants

Using lerna, I defined a local package named constants. I then tried to consume it in another lerna package via the statement const constants = require('constants'); Unfortunately, this pulls in the node.js internal constants module instead of my own package. This API is marked as a documentation-only deprecation, DEP0008. However, since it's a documentation-only deprecation, the node.js runtime is still reserving this module name for its own use. It would be great if this package were fully deprecated and the module name became available for consumers.

@vsemozhetbyt vsemozhetbyt added module Issues and PRs related to the module subsystem. deprecations Issues and PRs related to deprecations. labels Feb 8, 2019
@ChALkeR
Copy link
Member

ChALkeR commented Feb 8, 2019

I don't think that freeing up a package name for future (not existing) public use is a good reason for a complete removal of a built-in module.

@ChALkeR ChALkeR added the feature request Issues that request new features to be added to Node.js. label Feb 8, 2019
@ZebraFlesh
Copy link
Author

Using a monorepo tool such as lerna, each monorepo could declare a constants package for its own internal use. Lerna allows for package creation and resolution (via automation of symlinks) without requiring publication to a registry.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2019

I know it's not the solution that you're looking for, but you could use a relative file (require('./constants')) instead.

Even if we moved forward with the deprecation, I'm not sure that Node would release the name constants. See https://github.com/nodejs/node/blob/master/lib/sys.js

@cjihrig
Copy link
Contributor

cjihrig commented Feb 25, 2019

See the recent discussion in #26292 regarding the sys module. Specifically, #26292 (review), which applies to constants as well.

@ZebraFlesh
Copy link
Author

ZebraFlesh commented Feb 25, 2019

I know it's not the solution that you're looking for, but you could use a relative file (require('./constants')) instead.

That's not an option for me since I'm using Lerna. My require looks like require('constants'), but that module could be anywhere due to Lerna's symlinks. In the case I'm considering today, it happens to be in ../../lib/constants, but I could refactor my code and the import would be unchanged, again, due to Lerna's symlinks.

See the recent discussion in #26292 regarding the sys module. Specifically, #26292 (review), which applies to constants as well.

That's in the direction of helpful, but without the backing security discussion it's sort of just hand waving and saying "because of security". Where can I find the original discussion and decision? I see PR #2405 but it doesn't seem to include the discuss you referenced; that PR mentions a TC meeting from Dec. 30, 2014, but the notes don't discuss a security concern -- just adding a deprecation message.

@ZebraFlesh
Copy link
Author

ZebraFlesh commented Feb 25, 2019

Also, as someone writing application code that runs in the node runtime, where should I expect to see a deprecation message about constants? I don't recall seeing one, but it's entirely possible that node informed me I was doing something un-cool and I missed it.

edit:
Testing this out in a node repl session, I get:

> var foo = require('constants')
undefined
> var bar = require('sys')
undefined
> (node:8453) [DEP0025] DeprecationWarning: sys is deprecated. Use util instead.

So I'm thinking this means there is no deprecation message for constants in 10.15.1.

@jasnell
Copy link
Member

jasnell commented Feb 25, 2019

The reason constants is a docs-only deprecation is that there are still many modules out there using it, which makes a runtime-deprecation warning far more noisy that the relatively unused sys module. Also, even if the module were moved to End-of-Life, it would still need to be a reserved name in the ecosystem due to potential security issues.

For example, let's say we did drop constants from core. Existing code that today makes the correct assumption that require('constants') pulls in the deprecated module could unexpectedly get something else entirely in the future, without any indication that what they are loading is different.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 25, 2019

@ZebraFlesh As documentation states, it's currently a documentation-only deprecation.

Runtime deprecations are delayed when possible to ease the migration and not pollute user logs with the warnings that are meant for developers.
You can opt-in to see some of those by using the --pending-deprecation flag.
It currently does not include require('constants') though.

Btw, I think that that needs to be fixed by v12 (i.e. --pending-deprecation support for constants).
@jasnell, what do you think?

@jasnell
Copy link
Member

jasnell commented Feb 25, 2019

Upgrading to --pending-deprecation makes sense

@bnoordhuis
Copy link
Member

Where can I find the original discussion and decision?

The original original discussion was from before io.js, let alone the current Node.js project. I don't remember exactly when but that was at least five years ago.

The argument is simple: releasing a package name has only minor upsides but potentially serious downsides (think malicious npm modules.) It's easier to err on the side of caution.

@ZebraFlesh
Copy link
Author

--pending-deprecation will have to do, I guess. The user experience with require('sys') seems preferable because at least you receive some feedback that you're doing something bad/wrong/unsupported, but I guess that means moving constants from a doc-only deprecation to a runtime deprecation. Sounds like there's still more work to do to make that happen.

@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

There's been no further activity on this. Next step would be to open a PR adding the --pending-deprecation. Closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants