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

ESM support: allow package self-reference imports #10883

Closed
conartist6 opened this issue Nov 28, 2020 · 62 comments · Fixed by #12682
Closed

ESM support: allow package self-reference imports #10883

conartist6 opened this issue Nov 28, 2020 · 62 comments · Fixed by #12682

Comments

@conartist6
Copy link
Contributor

Node permits an es module inside a package to reference that package by its name. The behavior is documented here. I propose that jest also support this, as I believe the feature was created with testing in mind. It's highly useful for your internal tests to be able to reference your package in exactly the same way that external consumers will. I am willing to provide the implementation.

@SimenB
Copy link
Member

SimenB commented Nov 28, 2020

Yeah, we should implement all Node does for ESM. This seems dependent on #9771 landing first, tho?

@conartist6
Copy link
Contributor Author

Yep. No hurry, I have a workaround I'm using for now. Just wanted to ticket it so I would know when it was safe to remove the workaround.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Nov 28, 2020

This also applies to CommonJS modules: https://nodejs.org/api/packages.html#packages_self_referencing_a_package_using_its_name

@Lcfvs

This comment was marked as off-topic.

@conartist6

This comment was marked as off-topic.

@Lcfvs

This comment was marked as off-topic.

@ljharb

This comment was marked as off-topic.

@ljharb

This comment was marked as off-topic.

@Lcfvs

This comment was marked as off-topic.

@conartist6

This comment was marked as off-topic.

@Lcfvs

This comment was marked as off-topic.

@conartist6

This comment was marked as off-topic.

@Lcfvs

This comment was marked as off-topic.

@conartist6

This comment was marked as off-topic.

@conartist6

This comment was marked as off-topic.

@Lcfvs

This comment was marked as off-topic.

@ljharb

This comment was marked as off-topic.

@conartist6

This comment was marked as off-topic.

@conartist6

This comment was marked as off-topic.

@ljharb

This comment was marked as off-topic.

@conartist6

This comment was marked as off-topic.

@piranna
Copy link
Contributor

piranna commented Jan 22, 2022

Is there any update on this?

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2022

That is what I meant; that seems like a bug in node itself then.

@conartist6
Copy link
Contributor Author

Why would it be?? The node docs say "If you don't plan to publish your package, the name and version fields are optional." I interpret that to say quite clearly that any directory containing a package.json (even if it just contains {}) is a package. All of node's CLI commands operate on the nearest package by upward search, e.g. npm run script and npm publish.

What you seem to be saying is that some people might doing things like configuring resolver behavior for particular subdirectories by defining a package.json containing just:

{
   "type": "module"
}

OK, well, nobody can stop people from doing that if it works well enough for them, but that doesn't mean it's supported.

@conartist6
Copy link
Contributor Author

conartist6 commented Apr 12, 2022

I even just tried putting something completely invalid in package.json. npm still treats the directory containing the invalid package.json as a package, and just blows up when trying to work with it (as opposed to continue to search upwards that is, where there was a valid package.json present in the parent directory)

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2022

I'm saying exactly that - it's supported and it's what node's own docs recommend (i was on the node modules working group and was present for all those decisions)

@conartist6
Copy link
Contributor Author

conartist6 commented Apr 12, 2022

So what's the problem for self-refs then? If the nearest package.json file in the tree has a name, my understanding is that that would be the only module name that can be used as a self-reference.

I didn't know you were part that @ljharb. I have mad respect for how much you've done, and I depend on your work for my livelihood several times over.

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2022

I would expect, and hope, that node looks upwards until it finds a package boundary (a package.json next to the closest node_modules folder).

@conartist6
Copy link
Contributor Author

Why would a node_modules folder have anything to do with it? Packages do not necessarily have deps or devDeps.

My experimentation shows that only package.json#name has any bearing on self-refs, and that is exactly as I would expect and hope. Somehow we're not understanding each other here, but I can't tell how so yet.

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2022

OK, let me try again :-) does node skip a name-less package.json when looking to resolve a self-reference?

(The other question is: I believe self-reference respects "exports". But, node itself doesn't respect "exports" unless it's inside a node_modules package boundary. so i think that resolving a self-reference also requires "exports" support, which is why resolve doesn't have it yet)

@conartist6
Copy link
Contributor Author

conartist6 commented Apr 12, 2022

No, node never skips that I can tell.

Copying from the node package.json#export docs (in case anyone else ever reads this):

The "exports" field allows defining the entry points of a package when imported by name loaded either via a node_modules lookup or a self-reference to its own name. It is supported in Node.js 12+ as an alternative to the "main" that can support defining subpath exports and conditional exports while encapsulating internal unexported modules.

My example repo uses only exports, and shows node to be respecting them even without the presence of node_modules. However I also see that the main field is still the preferred entry point if it exists, unless we are resolving a reference into node_modules (or a self-reference). That's a bit weird (and the docs could be worded better), but I understand the need to preserve compatibility.

I don't think it's an issue here though since the behavior for self-references is specified, and jest-resolve uses the resolve.exports library to process exports itself:
https://github.com/facebook/jest/blob/59d3e51de6d21fdc0d9e0520f24144e1ec89e84c/packages/jest-resolve/src/defaultResolver.ts#L126-L139

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2022

Sorry to be stuck on this point - so you're saying that self-references inside a package only work when there's not an intermediate package.json file between the self-reference and the package's actual root? (ie, a file with just "type" or "main", but no "name")?

@conartist6
Copy link
Contributor Author

Yes, I am saying that.

@conartist6
Copy link
Contributor Author

Why does that surprise you? It seems to me to be wholly consistent with the other decisions you said you were involved in making -- decisions which to me amount to saying it will always be safe and correct to do a simple search upwards for a package.json file.

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2022

It surprises me because "exports" was specifically designed to only be respected at package boundaries. From the outside of a non-package-boundary directory, you can't require or import the contents of that directory in a way that respects "exports" (unlike "main").

@conartist6
Copy link
Contributor Author

conartist6 commented Apr 12, 2022

Well the moment you create a package.json you've created a package boundary haven't you? It's a bit squirrely since I have no idea what nested packages even really mean, but it's a boundary for the interior package. Since the prominent practical application of nested packages is monorepos it's usually the interior package that's the "real" package, and thus it's the one whose boundary we care about. But the same principle could then nest into itself again, making a package in a package in a package, and really it would be the innermost package that was "real" to files inside it, even if it had no name. But with no name, there would also be no way to access the package or its exports from the outside.

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2022

A package is something you can publish and put in another project's package.json - it's not demarcated merely by the presence of a package.json file.

@conartist6
Copy link
Contributor Author

conartist6 commented Apr 12, 2022

...so to sum up I'm not supposed to be able to make this exports object mean anything because this isn't a package in the technical sense (only in the sense of having a package.json file), but I can do an end-run around that by importing from a file inside the (inner) non-package package which uses a self-reference to access its otherwise-forbidden exports.

@conartist6
Copy link
Contributor Author

How bad a problem is that?

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2022

It will likely cause user confusion, and it makes things much much more complex for tooling (like jest, resolve, etc) for minimal benefit to the user.

@conartist6
Copy link
Contributor Author

I was a bit wrong actually -- you can't even self-reference a package with no name. It's just one of those things that you shouldn't do and it's not clear why you would. Add it to all the other ways you can hurt yourself if you don't know what you're doing.

As for "something you can publish and put in another project's package.json". Well, you don't have to have something publishable to have a usable "package", and in fact virtually anything can be put into another project's package.json. I just tried using "bitbucket": "/dev/null" as a dep and npm happily created node_modules/bitbucket as a symlink to /dev/null!

@SimenB
Copy link
Member

SimenB commented Apr 13, 2022

Lots of discussion here. 👍 Is the conclusion that for self reference, we can look to the closest package.json and if the name matches and there's exports, look those up? Otherwise use "normal" resolution? In that case, support is quite trivial in Jest I'd assume without looking too close at the code.

Question: what happens if both a module in node_modules and closest package.json has same name? I'd assume there's no node_modules check, but I've been surprised by node's resolution algorithm way too many times the last few years to trust my assumptions 😅

@conartist6
Copy link
Contributor Author

@SimenB That's my conclusion, and I'd think that self-refs would always have priority in the resolution process. If you meant to refer to your own package, you don't want changes in the runtime environment (e.g. additions to node_modules) to change the meaning of the code.

@SimenB
Copy link
Member

SimenB commented Apr 17, 2022

Yeah, playing in https://github.com/conartist6/node-self-ref-test I see

  1. bar passing, foo failing
  2. removing name from bar/package.json makes both fail
  3. deleting bar/package.json makes bar fail and foo pass

So to me logic look like

  1. find closest package.json
  2. if name matches and exports is present, use it (fail if subpath doesn't exist etc.. just normal exports)
  3. otherwise, go to normal node_modules resolution

@piranna
Copy link
Contributor

piranna commented Apr 17, 2022

I don't think exports should be check, but instead allows It to work with main field too.

@SimenB
Copy link
Member

SimenB commented Apr 17, 2022

Not from my testing, if main and no exports it's ignored.

https://nodejs.org/api/packages.html#self-referencing-a-package-using-its-name

Self-referencing is available only if package.json has "exports"

@SimenB
Copy link
Member

SimenB commented Apr 19, 2022

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants