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

Special treatment for package.json resolution and exports? #33460

Closed
1 task done
ctavan opened this issue May 18, 2020 · 100 comments · Fixed by gvergnaud/ts-pattern#97
Closed
1 task done

Special treatment for package.json resolution and exports? #33460

ctavan opened this issue May 18, 2020 · 100 comments · Fixed by gvergnaud/ts-pattern#97
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@ctavan
Copy link
Contributor

ctavan commented May 18, 2020

📗 API Reference Docs Problem

  • Version: 14.2.0
  • Platform: any
  • Subsystem: esm

Location

Section of the site where the content exists

Affected URL(s):

Problem description

Concise explanation of what you found to be problematic

With the introduction of pkg.exports a module only exports the paths explicitly listed in pkg.exports, any other path can no longer be required. Let's have a look at an example:

Node 12.16.3:

> require.resolve('uuid/dist/v1.js');
'/example-project/node_modules/uuid/dist/v1.js'

Node 14.2.0:

> require.resolve('uuid/dist/v1.js');
Uncaught:
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './dist/v1.js' is not defined by "exports" in /example-project/node_modules/uuid/package.json

So far, so good. The docs describe this behavior (although not super prominently):

Now only the defined subpath in "exports" can be imported by a consumer:

While other subpaths will error:

While this meets the expectations set out by the docs I stumbled upon package.json no longer being exported:

> require.resolve('uuid/package.json');
Uncaught:
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './package.json' is not defined by "exports" in /example-project/node_modules/uuid/package.json

For whatever reason I wasn't assuming the documented rules to apply to package.json itself since I considered it package metadata, not package entrypoints whose visibility a package author would be able to control.

This new behavior creates a couple of issues with tools/bundlers that rely on meta information from package.json.

  • So right now, packages that do contain information to be consumed externally (e.g. by bundlers) in their package.json have to add the package.json to the pkg.exports field.
  • On the other hand bundlers/etc should then handle the case where a dependency doesn't export the package.json gracefully, since it might very well be the fact that a given package doesn't need to export any bundler meta information (and otherwise almost all packages on npm that could ever be used in a react-native project would have to add package.json to their exports).
  • If bundlers however handle a missing package.json exports gracefully, I see the risk that many modules that currently rely on their package.json simply being externally consumable without additional effort might suddenly behave in odd ways when the meta information from package.json is no longer readable by bundlers (and no error is thrown).

Examples where this issue already surfaced:

Now the question is how to move forward with this?

  1. One option would be to keep the current behavior and improve the documentation to explicitly warn about the fact, that package.json can no longer be resolved unless added to exports. EDIT: Already done in 1ffd182 / Node.js v14.3.0
  2. Another option would be to consider adding an exception for package.json and always export it.

I had some discussion on slack with @ljharb and @wesleytodd but we didn't come to an ultimate conclusion yet 🤷‍♂️ .


  • I would like to work on this issue and submit a pull request.
@ctavan ctavan added the doc Issues and PRs related to the documentations. label May 18, 2020
@ljharb
Copy link
Member

ljharb commented May 18, 2020

cc @nodejs/modules-active-members

@wesleytodd
Copy link
Member

other option would be to consider adding an exception for package.json and always export it.

To me this seems like a great solution. Having the package metadata is an awesome ergonomic feature of the current setup, and having module authors explicitly have to opt in would be a huge burden across the community. To me it seems like we would need a concrete reason not to have this exception. Can anyone think of a reason to make this case?

@bmeck
Copy link
Member

bmeck commented May 18, 2020

@wesleytodd I think it just comes down to what is public/private still. People putting public data into a a package.json for things like tools to consume isn't really an issue. People putting configuration data like secrets is more the concern. I imagine it would still be able to be censored if people re-wrote the import still.

However, I'm unclear on the privacy model here since the benefit seems largely to be around bundlers which wouldn't run with the same constraints since they are ahead of time tools and general thought to access things in a more permissive manner than 2 independent modules with mutual distrust. It seems the problem is in part that these tools are using APIs that make them act as the same level of trust as any other module and other packages when they upgrade are removing permissions to view the package.json data (even if by accident). I think the concrete discussion here is if people should have to opt-out of package.json to avoid an accident prone workflow which is the inverse of all other resources in the package.

A different option since there is a specific use case that seems to need this is to have a flag of some kind for these ahead of time tools. Either to require.resolve which looks to be the cause of issues above, to node via CLI/ENV, or something else. I do think providing an exception would make things just work, but somewhat go against the privacy intent of "exports".

@jkrems
Copy link
Contributor

jkrems commented May 18, 2020

My main concern with exposing it via exports is that there’s two options:

  1. Make it non-configurable. ./package.json always has to map to ./package.json.
  2. Tools that use require or import to load metadata may get custom files because a package decided to remap ./package.json.

The first option would force every package to treat its metadata file as a public API. Some users eslint config reuses what I put into my package.json#eslint section? Well, it’s exported so how can I blame them.

The second option means that tools may not actually get the metadata when they think they’re loading the metadata. It could be argued that it only affects “weird” packages but given a sufficiently large number of users that know about this “trick”, I can totally see people use it.

I think bundlers shouldn’t use require (“load code for this environment”) to load metadata. So I’d rather have a new API to load the package.json that belongs to a specifier/referrer combination, exposing logic we already have in the loader. That API could be used by bundlers etc to cleanly get the metadata without having to hijack require.

@ljharb
Copy link
Member

ljharb commented May 18, 2020

I think the primary issue is that there's no way besides require.resolve to resolve the package.json for a package. Even path.join(require.resolve(pkg), 'package.json') won't work because some packages might have their "main" resolve to a subdirectory.

@guybedford
Copy link
Contributor

guybedford commented May 18, 2020

Here's the recommended way to do this with ES modules:

import { readFileSync } from 'fs';
(async () => {
  const pkgPath = await import.meta.resolve('pkg/')
  console.log(pkgPath);
  console.log(readFileSync(new URL('package.json', pkgPath)).toString());
})();

The above also simplifies with TLA of course.

Currently the above only executes with --experimental-import-meta-resolve.

@nodejs/modules-active-members I think we should discuss unflagging this feature.

@ctavan
Copy link
Contributor Author

ctavan commented May 18, 2020

I also want to clarify that the problems I have seen in the wild were always just about require.resolve('pkg/package.json') in order to then load that file from the filesystem. I didn't see anybody trying to directly require('pkg/package.json') to really load the json data as a module.

@ljharb
Copy link
Member

ljharb commented May 18, 2020

@guybedford import.meta.resolve('pkg/') would fail if the package didn't have a main/dot, wouldn't it? or, if the ./ was mapped to a different directory, like ./src?

@guybedford
Copy link
Contributor

@ljharb no, the trailing / is specially specified to allow resolving package boundaries.

@ljharb
Copy link
Member

ljharb commented May 18, 2020

@guybedford require.resolve('es-get-iterator/') in latest node throws Package exports for '$PWD/node_modules/es-get-iterator' do not define a './' subpath. I would expect import.meta.resolve to behave identically, so the same capabilities exist in both CJS and ESM.

iow, import.meta.resolve only solves for ESM, not CJS, so it's not a solution to this problem.

@guybedford
Copy link
Contributor

@ljharb yes, because trailing slashes in CommonJS still apply extension searching, which the ESM resolver does not, which is a fundamental difference between the module systems.

@wesleytodd
Copy link
Member

wesleytodd commented May 18, 2020

People putting configuration data like secrets is more the concern (@bmeck)

This is a not a concern. Using exports to hide secrets is not ever a reasonable solution anyway.

Make it non-configurable. ./package.json always has to map to ./package.json. (@jkrems)

This is what I was thinking as well. Seems perfectly reasonable to enforce this constraint.

The first option would force every package to treat its metadata file as a public API. (@jkrems)

It already is. This is not a change in the ecosystem as it is today. Every file is part of the public api, and needs to be treated as such. If projects choose not to strictly follow semver, that is a different issue.

Here's the recommended way to do this with ES modules: (@guybedford)

We can also use hacks around require for this. The point is that the most ergonomic way is also popularly in use, so should be added as an exception to the exports spec, even if there are other ways around it.

I didn't see anybody trying to directly require('pkg/package.json') to really load the json data as a module. (@ctavan)

I have seen this many places. Although I am not going to spend the time now collecting references since I don't think this should be the primary focus of the discussion, if it become a key point I am happy to dig for them.

@bmeck
Copy link
Member

bmeck commented May 18, 2020

I think allowing censorship is necessarily good and wouldn't feel comfortable with ./package.json always mapping to ./package.json would not seem to allow that. In particular, people do set environment variables in their ./package.json when deploying things in various environments that support it. Environment variables might be able to be removed at runtime via process.env but if the deployment does not have a writable filesystem they could not censor their package.json. I am not really here to judge if this workflow is a good idea, just to note that it does present a concern for myself.

@wesleytodd
Copy link
Member

wesleytodd commented May 19, 2020

I think allowing censorship is necessarily good and wouldn't feel comfortable with ./package.json always mapping to ./package.json would not seem to allow that.

Making it more complicated for everyone seems to strongly outweigh this. I know the point of engines is to help package authors have more explicit contracts with their users, but if this just breaks everyone's tooling it is a net negative to the community, especially for the package authors who now have to deal with this added complexity.

I am not really here to judge if this workflow is a good idea, just to note that it does present a concern for myself.

Do you have examples of this type of workflow? The app developer use case is not what I was considering at first, so maybe there is some common practice I have not seen like this. If so we would not want to break it. That said, I feel like the current state before exports had no restrictions on this, so I am not sure how we would be making anything worse.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

I'm confused about why this is a concern; if you put secrets in a place on the filesystem that the node user can access, your secrets are already exposed. exports is not a security feature, as we discussed many times during its development.

@guybedford
Copy link
Contributor

I think the problem is more about making the package.json file part of the public API of a package.

The goal of exports is to fully encapsulate the public API of a package in a way that allows sound analysis of execution, optimization, breaks etc etc.

Exposing the package.json goes against this by making the properties of the package.json part of the public API.

There are many ways to access the package.json otherwise - you are not stopped from doing it, it just takes a little more code. Updating require.resolve patterns to a fs.readFile pattern is all it is.

Also note that this mostly applies far more to frameworks than libraries. Frameworks can at least take the effort to understand the problem here and fix the root cause I'd hope.

@wesleytodd
Copy link
Member

wesleytodd commented May 19, 2020

Exposing the package.json goes against this by making the properties of the package.json part of the public API.

I think the goal would be to explicitly document this fact (and codify it as part of the implementation). Just call it part of the public api, always and forever, and be done with it. And to be clear, adding exports broke the existing behavior which was that all files in a package are part of their public api. So going back on one clearly good exception seems to be a more reasonable middle ground than breaking every tool which relies on this today.

Also note that this mostly applies far more to frameworks than libraries.

Not sure I understand the distinction here. I have libraries which load package.jsons to inspect them via require.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

That’s the problem - you can’t update to a readFile pattern if you can’t get the path to the file robustly, in CJS. That’s not possible right now for a package with exports, that doesn’t include package.json, and whose dot/main either is set to false or points to a subdir.

@rektide
Copy link

rektide commented May 19, 2020

This focus around package.json seems incidental to me. As a user, I would very much like to be able to require()/import items from the file system, which is the most apparent & comprehensive truth to me.

That this is not longer possible if there is a pkg.exports seems like a very critical degredation of what I as a consumer of modules would hope & desire. If package.json exports do export something, fine, I'll take that, but I should continue to be able to require/import files that a package distributes. Including package.json.

I beg node to please adjust course & not hide the file system the moment an author declares a package.json exports.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

That’s the entire purpose of “exports”, and it’s a highly desired one - that’s not something that we’re discussing here.

@rektide
Copy link

rektide commented May 19, 2020

That’s the entire purpose of “exports”, and it’s a highly desired one - that’s not something that we’re discussing here.

well where do we discuss it jordan, because it's a bad choice & confusing for everyone? there should be room to fallback into actual real resources if not defined in this new abstract package.json system node invented for itself.

i don't see why we shouldn't have both. it would solve this issue. it would allow people who have for a decade now required()'d resources continue to do so when their package authors miss this or that resource. i think the package consumers deserve more than they are getting with this "highly desired" system.

@guybedford
Copy link
Contributor

@rektide the full resources are still available at require('/absolute/path/to/package.json') exports only provides a filtering when entering the package through the public interface, via require('pkg/subpath').

If the problem is how to resolve the package path without having a suitable subpath, this is what the trailing slash was designed to allow in the example provided at #33460 (comment).

If you don't like change, don't adopt exports.

@guybedford
Copy link
Contributor

guybedford commented May 19, 2020

@ctavan you make a good point in #33460 (comment). Perhaps one option could be to treat package.json as an exception in require.resolve ONLY (and not for require), where on a PACKAGE_PATH_NOT_EXPORTED error an internal fallback resolution approach applies.

I would not want such a path implemented for import.meta.resolve though.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

Anything require.resolve resolves must also be obtainable via require, otherwise the entire thing doesn't make sense.

@guybedford
Copy link
Contributor

Having it just for require.resolve would definitely be an inconsistency in the name of backwards compat practicality, yes.

@dipendra-sharma
Copy link

dipendra-sharma commented Jul 18, 2022

One more.

react-native-community/cli#1650

Conaclos added a commit to bare-ts/bare that referenced this issue Jun 1, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 1, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 8, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 8, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/bare that referenced this issue Jun 8, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/bare that referenced this issue Jun 10, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/bare that referenced this issue Jun 11, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/bare that referenced this issue Jun 11, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 11, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/bare that referenced this issue Jun 14, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 19, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 19, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 19, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 19, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/bare that referenced this issue Jun 19, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/lib that referenced this issue Jun 19, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
Conaclos added a commit to bare-ts/bare that referenced this issue Jun 19, 2023
To ensure that `package.json` is qccessible, it has to be exported.
See nodejs/node#33460

For the same reason, we now export TypeScript types.
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.