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

add API for "package dir" #49445

Open
ljharb opened this issue May 19, 2020 · 72 comments
Open

add API for "package dir" #49445

ljharb opened this issue May 19, 2020 · 72 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.

Comments

@ljharb
Copy link
Member

ljharb commented May 19, 2020

See #33460.

tl;dr: it turns out that there's a number of tools that need to be able to locate a package's root directory, and with "exports", they can no longer rely on package/package.json working, nor package/.

If we could provide this API, then CJS could path.join it, and ESM could use URL, to get whatever file the tool needs to fs.readFile.

Adding this new API would bypass questions about "should package.json be an implicit part of a package's API", as well as avoid reliance on ESM-only or CJS-only mechanisms. By providing the package root dir rather than its package.json, we would not be encouraging or discouraging any patterns of "where to store metadata" - instead, we'd be correctly leaving that choice up to userland.

Example solution: module.packageDir(specifier) and module.packageDirSync(specifier) (sync access is critical for many use cases).

@ljharb ljharb added modules-agenda Issues and PRs to discuss during the meetings of the Modules team. esm Issues and PRs related to the ECMAScript Modules implementation. labels May 19, 2020
@coreyfarrell
Copy link
Member

How would the resolver source be determined? Maybe require.packageDir would be a better place for the API so could have access to the callers __filename? For example you have the following packages installed:

node_modules/pkg1
node_modules/pkg1/node_modules/pkg2
node_modules/pkg2

node_modules/pkg1/index.js asks for packageDir of pkg2, it needs to find the nested one.

@ljharb
Copy link
Member Author

ljharb commented May 19, 2020

Yes, that is true. It would need to do the searching contextually, just like require.resolve, so require.packageDir would be a better place to put it (and thus perhaps, for ESM, import.meta.packageDir)

@GeoffreyBooth
Copy link
Member

As paths are URLs in ESM, "dir" is perhaps not the best name. But aside from naming, yes, this would be a good method to expose. In #33460 (comment) I had suggested resolvePackageRoot.

@guybedford
Copy link
Contributor

I'm removing the modules agenda label here, as the discussion around this seems to be the same topic as #33460 which is still on the agenda as well.

@stevenvachon
Copy link

Any plans to resolve this?

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2021

That seems a bit overkill.

All that I think is needed is Module.getPackageDir(packageSpecifier), and then anything needed from there can be done with fs APIs.

I haven't yet had time to make a PR, but anyone is welcome to beat me to it.

@bmeck
Copy link
Member

bmeck commented Feb 28, 2021

Module.getPackageDir(packageSpecifier) would need a specifier to know where it is starting resolution.

I also am unclear on how any API should integrate with redirection mechanisms like policies and import maps. I'd personally be fine ignoring redirection mechanisms but that could lead to problems where importing doesn't match the API.

@targos
Copy link
Member

targos commented Feb 28, 2021

We could add a method to the require function so it knows where to start resolution.

@targos
Copy link
Member

targos commented Feb 28, 2021

Or a function that takes two parameters. getPackageDir(__filename, packageSpecifier) in CJS and getPackageDir(import.meta.url, packageSpecifier) in ESM.

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2021

I’d indeed expect a second argument to the static method that took __dirname/import.meta.url. Dangling it off require wouldn’t work in ESM.

I’m not sure how it would interact with policies, but i don’t think there’s any security issues from retrieving a path - if filesystem access is restricted, then you’d be unable to use it, which policies could dictate?

@bmeck
Copy link
Member

bmeck commented Feb 28, 2021

@ljharb I am unclear / don't see an issue at a glance, but the problem can be explained by example:

// redirected to file:///app/node_modules/apm/interceptions/sql.js instead of file:///app/node_modules/sql/index.js
import.meta.resolve('sql');
// likely want file:///app/node_modules/sql/ (trailing slash, yes) ? but that now doesn't match where it is actually getting loaded from
module.package('sql');

This would affect pretty much any custom resolver the more I think about it, but we tend to state for policies that it is the job of the policy creator to manually enforce things like fs access per callsite (by redirecting as desired per scope) and similarly we can just state they need to customize module as well. This is a problem with a single argument form if it isn't an absolute location (path or URL) since it cannot be customized per scope. Similarly, without an absolute location, it wouldn't know what to resolve against, it could use process.cwd() but then things in node_modules can get the wrong values.

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2021

Makes perfect sense. Theoretically, this api in node could use the same resolver that would respect policies, and could return the directory that contains that package's package.json - then it wouldn't require extra work on behalf of the policy creator.

The double argument form, with source location, was always the plan; i just failed to mention the second argument in my above comment.

@bmeck
Copy link
Member

bmeck commented Feb 28, 2021

@ljharb if it respects policies, are you saying it should return file:///app/node_modules/apm/? since that is the package for the resolved "sql"? Policies are not realistically human writable (same as any non-trivial import map) so I wouldn't worry too much about work on creating since it can be left to tooling. Policies could always add data for this method to use rather than trying to make it work off the actual resolved location.

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2021

It should return whatever path foo would make fs.readFileSync(path.join(foo, 'package.json')) return the expected package.json contents (presuming fs access is available).

@bmeck
Copy link
Member

bmeck commented Feb 28, 2021

@ljharb I'm asking which package.json is expected

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2021

That of the sql package, in this case. If the apm wants to provide a fake package dir with a custom package.json it is certainly free to construct one.

@bmeck
Copy link
Member

bmeck commented Feb 28, 2021

@ljharb ah, file:///app/node_modules/sql/ in the example above then; by default it wouldn't resolve using the result of the current data in policies and a new data point would be needed to intercept that (same for loading hooks).

@ljharb
Copy link
Member Author

ljharb commented Mar 11, 2021

precise naming aside, that's the only API we've ever really discussed or expected.

@VincentBailly
Copy link

@MylesBorins what is your opinion here, is it okay to ship an API that does not respect policies?

@MylesBorins
Copy link
Contributor

I would not personally block on policies support but if @bmeck or others would then it would be a requirement.

@bmeck does require.resolve respect policies?

@bmeck
Copy link
Member

bmeck commented Apr 27, 2021

@bmeck does require.resolve respect policies?

Currently, no. Either way is fine, but I'd prefer we not make it align with require() in that sense. Interestingly import.meta.resolve does match import though so this likely should be made to align either way. Slight preference on making both resolve and loading the same as it makes the implementation simpler.

@ljharb
Copy link
Member Author

ljharb commented Apr 27, 2021

I think module features should always align with require and import, and when require/import aligns with policies, this should too. If those are inconsistent with alignment to policies, that seems like a bug.

@bmeck
Copy link
Member

bmeck commented Apr 27, 2021

@ljharb I can make them align. If we do though, the situation described above doesn't seem to be what you want?

@MylesBorins
Copy link
Contributor

It seems like what we should do is kick off a PR to core and work these out there. I think if we mark this as an experimental API we could always land something with a warning and gather feedback

@ljharb
Copy link
Member Author

ljharb commented Apr 27, 2021

@bmeck basically the entire purpose of this API is to provide a reliable replacement for require.resolve(path.join(packageName, 'package.json')) even in the presence of “exports”; if policies hijack a module and point it somewhere else, I’d assume either all of, or none of, requires, imports, and this API would follow suit.

@bmeck
Copy link
Member

bmeck commented Apr 27, 2021

@ljharb ok, can do

@MylesBorins
Copy link
Contributor

I would add... if we add this API as experimental we could align everything (import / require / resolve / this api) as a single push... I don't think we should block an initial implementation on a larger platform inconsistency

This comment has been minimized.

Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically 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 Aug 28, 2024
@ljharb
Copy link
Member Author

ljharb commented Aug 28, 2024

never stale.

@github-actions github-actions bot removed the stale label Aug 29, 2024
@thescientist13
Copy link

Just wanted to chime in that this would be really helpful as I have a use case where I am also trying to get to the location of a package's package.json and was using import.meta.resolve to get there, but bumped into a package (@types/trusted-types) which has an empty main field in its package.json which causes import.meta.resolve to throw an ERR_MODULE_NOT_FOUND error

➜  import-meta-resolve-demo git:(master) ✗ npm run demo

> import-meta-resolve-demo@1.0.0 demo
> node .

✅ SUCCESS: resolved lit at location =>  file:///Users/owenbuckley/Workspace/github/import-meta-resolve-demo/node_modules/lit/index.js
🚨 ERROR: could not resolve @types/trusted-types Error: Cannot find package '/Users/owenbuckley/Workspace/github/import-meta-resolve-demo/node_modules/@types/trusted-types' imported from /Users/owenbuckley/Workspace/github/import-meta-resolve-demo/index.js
    at legacyMainResolve (node:internal/modules/esm/resolve:204:26)
    at packageResolve (node:internal/modules/esm/resolve:827:14)
    at moduleResolve (node:internal/modules/esm/resolve:907:18)
    at defaultResolve (node:internal/modules/esm/resolve:1037:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:650:12)
    at #cachedDefaultResolve (node:internal/modules/esm/loader:599:25)
    at #resolveAndMaybeBlockOnLoaderThread (node:internal/modules/esm/loader:615:38)
    at ModuleLoader.resolveSync (node:internal/modules/esm/loader:632:52)
    at Object.resolve (node:internal/modules/esm/initialize_import_meta:33:25)
    at file:///Users/owenbuckley/Workspace/github/import-meta-resolve-demo/index.js:6:34 {
  code: 'ERR_MODULE_NOT_FOUND'
}

You can see a repro repo here - https://github.com/thescientist13/import-meta-resolve-demo

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.
Projects
Status: Awaiting Triage
Development

No branches or pull requests