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 package.json to exports #1

Closed
wants to merge 1 commit into from

Conversation

andrewbranch
Copy link

@arethetypeswrong/history includes the npm-high-impact version used to generate its data, so being able to access the package.json via import/require is convenient 😄

Signed-off-by: Andrew Branch <[email protected]>
Copy link

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All packages need to provide this, or tooling breaks.

@wooorm
Copy link
Owner

wooorm commented Oct 31, 2023

Heya!

I dunno, I don’t feel package.jsons are part of the public API covered by semver, which is what I use export maps for. Should I bump a major when I change an npm script name?

I have personally been doing things like:

import fs from 'node:fs/promises'

const pkgUrl = new URL('package.json', import.meta.resolve('bare-specifier'))
const pkgJson = JSON.parse(String(await fs.readFile(pkgUrl)))

console.log(pkgJson)

I’m pragmatic about it though, if you need it we can add it!

All packages need to provide this, or tooling breaks.

I have 100s of packages without package.json in export map and I haven’t seen tooling break personally.

@ljharb
Copy link

ljharb commented Oct 31, 2023

That technique can’t work 100% of the time; the bare specifier doesn’t necessarily resolve to the package directory.

@wooorm
Copy link
Owner

wooorm commented Oct 31, 2023

It was an example, a variable, bare-specifier/package.json doesn’t always work either, but npm-high-impact/package.json does

@ljharb
Copy link

ljharb commented Oct 31, 2023

The former does always work prior to exports, which is why adding it in explicitly is helpful - because it’s what everything assumes will work.

@wooorm
Copy link
Owner

wooorm commented Oct 31, 2023

I have no clue what you mean.

@ljharb
Copy link

ljharb commented Oct 31, 2023

The point of the exports field is just to make explicit which files are accessible, which isn’t the same thing as defining your API. Prior to that field existing, you could always require bare-specifier/package.json, so all tools for the first decade of node have assumed that will work, and used it over fs APIs. With the advent of the exports field, there’s no longer a reliable way that works for every package to get at the package.json - so, if you add it to exports as this PR does, then you’ve restored the path that every tool already assumes will work, and there’s no other reliable workaround anyways (ie, no official “get the package dir” node api)

@wooorm
Copy link
Owner

wooorm commented Oct 31, 2023

Right, the export field is different, I like these changes, that’s why I use it.
I use export fields to close the API down and don’t use “paths” or “extensions” in them.
If I’d add package.jsons everywhere as this PR asks for, I’d not use .json, as it’s misleading, but go with something like npm-high-impact/package.

@wooorm
Copy link
Owner

wooorm commented Oct 31, 2023

So like, which automated tools does not having a package.json export break? I have not received issues about this. (and this is different from the PR here, which can be solved with #1 (comment))

@andrewbranch
Copy link
Author

In my mind, the API contract around exposing a package.json is limited to

  • it is resolvable
  • it is JSON.parseable
  • Possibly it has the fields required for publishing to npm if I installed it from npm?

And otherwise the content is not part of the API. But as far as I know, this wasn’t written down somewhere; it just feels reasonable to me. If you disagree, no worries; of course I can work around it in the way you showed. I opened the PR because my project already was using createRequire(import.meta.url)("npm-high-impact/package.json"), but it broke in a minor version after 079807b. So this is an instance of “tooling breaking” I guess, though it’s in a build script, so users aren’t affected.

Happy to work with whatever you decide here.

@ljharb
Copy link

ljharb commented Oct 31, 2023

Why would .json be misleading? That's what it is.

@wooorm
Copy link
Owner

wooorm commented Oct 31, 2023

but it broke in a minor version after 079807b. So this is an instance of “tooling breaking” I guess, though it’s in a build script, so users aren’t affected.

Ah, whoops, sorry about that. I’ve been doing majors everywhere but I guess I forgot that here.

Why would .json be misleading? That's what it is.

In this case it is. In other cases it isn’t. Such as this case where TS incorrectly thinks .json means JSON: https://github.com/wooorm/starry-night/blob/fc3e7b3352c4037a3ec0edd5ce9a2209b2ad30db/lib/common.js#L56-L57


Does TS actually do things with JSON files? Assuming a xxx.json on disk or an import obj from "yyy" with { type: "json" }? 🤔

@ljharb
Copy link

ljharb commented Oct 31, 2023

When the tsconfig option is set (as init sets it), I believe it works just as it works with CJS.

@wooorm
Copy link
Owner

wooorm commented Nov 6, 2023

Thanks for the PR and the discussion!
I think this should be added to Node instead of that every package with exports should include it (and type it, and document it, and choose how to name it). There’s an issue already nodejs/node#49445. Luckily, Node already has all the code for this internally.
I just this question on import-meta-resolve, and due to the Node issue existing, I think I’ll just land it there: wooorm/import-meta-resolve#18

And sorry that I pushed this change in a minor instead of a major!

@wooorm wooorm closed this Nov 6, 2023
@andrewbranch andrewbranch deleted the patch-1 branch November 6, 2023 17:17
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.

3 participants