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

Update package.json to exports itself #243

Merged
merged 2 commits into from Dec 2, 2020

Conversation

Graphmaxer
Copy link
Contributor

@Graphmaxer Graphmaxer commented Nov 17, 2020

This change is needed to access the package.json for external bundlers as documented here and discussed here. I also did the same for another npm package here.

@coveralls
Copy link

coveralls commented Nov 17, 2020

Pull Request Test Coverage Report for Build 331e3533a24f2ead6c2d9986b725ca42cbeefafc-PR-243

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build e329ed55f00f416e335b3e8bfcd7b3ec728186cb: 0.0%
Covered Lines: 236
Relevant Lines: 236

💛 - Coveralls

@Andarist
Copy link
Collaborator

Why would you like to import Stylis' package.json though?

@Graphmaxer
Copy link
Contributor Author

Graphmaxer commented Nov 17, 2020

It's not for my usage but for bundlers, for example with rollup and svelte, the bundler want to access the package.json to check for a custom property named svelte. There is also the same for React Native as discussed here and here.

@Andarist
Copy link
Collaborator

Oh, gotcha. I don't think this is a scalable solution though - they should switch to fs APIs.

@Graphmaxer
Copy link
Contributor Author

Graphmaxer commented Nov 17, 2020

I agree, this current situation is not ideal and that's why the node.js team is thinking about auto-exporting the package.json for all packages as discussed here. @ljharb is working on it.

@Andarist
Copy link
Collaborator

Btw. from what I remember Svelte's compiler is Rollup-based and it seems that @rollup/plugin-node-resolve is using FS APIs to access package.json: https://github.com/rollup/plugins/blob/3d60158f21e0b6a5a8a53d9977928e2e148cb885/packages/node-resolve/src/resolveImportSpecifiers.js#L133-L134

@Graphmaxer
Copy link
Contributor Author

Graphmaxer commented Nov 17, 2020

You're right this should work with the @rollup/plugin-node-resolve but svelte is using rollup-plugin-svelte to compile svelte via rollup and here is where they access the package.json. In this code it use relative.resolve with /dist (value of path.dirname(importer)), that's why it cannot access the package.json file.

@thysultan
Copy link
Owner

package.json to check for a custom property named svelte

Is this specific to svelte only? Because stylis doesn't export a svelte pkg.json field making this issue moot?

@Graphmaxer
Copy link
Contributor Author

Stylis don't need to modify its package.json file, it just need to exports it because it was a default behavior in the past and now it needs to be manual.

@Andarist
Copy link
Collaborator

Andarist commented Dec 1, 2020

@thysultan the problem is only that Svelte's compiler is trying to require the stylis/package.json because it wants to check if there is a custom svelte key there but node rejects their request because we have exports map defined and we do not allow for stylis/package.json to be accessed within it. This is a problem with their approach to read package.json - they really should FS APIs to read this file (which wouldn't be rejected) instead of a plain require

@thysultan
Copy link
Owner

I'd imagine many other packages would have this same issue, @Graphmaxer can you file this issue with the svelte compiler.

Thanks.

@thysultan thysultan closed this Dec 1, 2020
@Graphmaxer Graphmaxer deleted the patch-1 branch December 1, 2020 13:19
@ljharb
Copy link

ljharb commented Dec 1, 2020

@thysultan any other package using “exports” but not explicitly including package.json does have this issue, and the fix in this PR is the only solution available. The svelte compiler has no reliable way to do this otherwise.

@Graphmaxer Graphmaxer restored the patch-1 branch December 1, 2020 15:25
@Andarist
Copy link
Collaborator

Andarist commented Dec 1, 2020

@ljharb Why using FS APIs is not a possible solution for the Svelte compiler? @rollup/plugin-node-resolve does exactly this and it doesn't suffer from this issue:
https://github.com/rollup/plugins/blob/3d60158f21e0b6a5a8a53d9977928e2e148cb885/packages/node-resolve/src/resolveImportSpecifiers.js#L133-L134

@ljharb
Copy link

ljharb commented Dec 1, 2020

It’s impossible to reliably know where package.json lives on the filesystem. That plugin is using heuristics to figure out the package name, which does not reliably locate the proper package.json in every case.

As a member of the node modules team and the maintainer of https://npmjs.com/resolve, i can assure you that the only fix (prior to node itself adding an api for locating a package directory) is for every individual package using “exports” to also add package.json to it.

@Andarist
Copy link
Collaborator

Andarist commented Dec 1, 2020

I know about your involvement in those projects but I would also like to understand the issue at hand (so for example I could explain it to others in the future). I fail to see when this heuristic could fail.

According to the https://nodejs.org/api/esm.html#esm_resolver_algorithm and the PACKAGE_RESOLVE algorithm .exports is always read from pjson that is a result of READ_PACKAGE_JSON (step 10.5). So if I understand this correctly that READ_PACKAGE_JSON algorithm would have to return something that doesn't fit that simple heuristic. That algorithm mentions this:

Let pjsonURL be the resolution of "package.json" within packageURL.

But I can't find anything beyond that and this statement doesn't seem to tell me on its own how this is resolved.

Also, the call to READ_PACKAGE_JSON looks like this: READ_PACKAGE_JSON(packageURL) (step 10.4) so either way this is resolved from the "root" of the package so how this could return anything beyond lib/package.json?

@ljharb
Copy link

ljharb commented Dec 1, 2020

The issue is exactly that - when an exports dot points to a file in a dir, that in turn has a package.json.

Certainly tools could reimplement node’s own require algorithm, as resolve itself has, but this is far more complicated than packages adding a single line to their own exports object.

@Andarist
Copy link
Collaborator

Andarist commented Dec 1, 2020

The issue is exactly that - when an exports dot points to a file in a dir, that in turn has a package.json.

I don't follow. Could you illustrate this with a simple example?

Certainly tools could reimplement node’s own require algorithm, as resolve itself has, but this is far more complicated than packages adding a single line to their own exports object.

Well, at least if tools do this then this complexity lies just in a couple of tools whereas if the community will be forced to add package.json to the exports map the "complexity" gets spread across the ecosystem. I strongly believe that this solution just doesn't scale at all and adding this to packages is just counter-productive.

@ljharb
Copy link

ljharb commented Dec 1, 2020

The longer term solution is to add an api to node itself to expose the package dir, which tools can then use. That solution is not yet ready, and in the interim, a package’s choices are “don’t use exports”, or “add package.json”, or “screw over users by breaking the majority of tools that don’t yet use fs APIs or resolve”.

@Andarist
Copy link
Collaborator

Andarist commented Dec 1, 2020

I find it hard to blame package authors for "screwing over users" if this has not been accounted for by the node team... If anything I feel like this should be prioritized there so we wouldn't have to do anything for a short period of time in which this stays "bugged" on the node's side.

That being said I still have no idea how a package can be structured right now that would be incorrectly handled by the @rollup/plugin-node-resolve 😕

@ljharb
Copy link

ljharb commented Dec 1, 2020

That is a reasonable reaction, and i don’t dispute it - but that doesn’t change the reality now.

I’d have to take a look at its logic to locate bugs, but i don’t pay much attention to the rollup ecosystem, so I’m not sure when I’d find time.

@thysultan thysultan reopened this Dec 2, 2020
@Graphmaxer
Copy link
Contributor Author

@thysultan Thanks for reopening this. I just merged master to resolve conflicts.

@thysultan thysultan merged commit e8afd5c into thysultan:master Dec 2, 2020
@Graphmaxer Graphmaxer deleted the patch-1 branch December 2, 2020 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants