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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolver bugfixes #1686

Merged
merged 11 commits into from
Dec 9, 2023
Merged

Resolver bugfixes #1686

merged 11 commits into from
Dec 9, 2023

Conversation

mansona
Copy link
Member

@mansona mansona commented Nov 28, 2023

(Edits by @ef4:) This PR now encompasses several bug fixes. We made them all here together because their impact was inter-dependent -- fixing one revealed the others as failing tests.

  • the node resolver now supports package.json exports by switching from the resolve package to require.resolve. This needed some finesse because require.resolve has some quirky behaviors.
  • the emberVirtualPeerDeps can now find addons that are not directly resolvable from the app
  • the webpack resolver was leaking state between requests and that is now fixed
  • the active addons fallback now also supports the app as a target (since some people do resolve the app by name from addons)
  • it was possible for requests to escape the moved package system when our node resolver and the prevailing packager's resolver disagreed. Now requests that we think should fail are forced to fail, allowing them to progress correctly down the rest of the fallbacks.

Original PR description follows:

Since we merged #1648 we can now have situations where our resolver requires you to rely on the exports field of dependencies to correctly locate a file. This is totally fine in our Webpack build, but with Vite and Esbuild you can have cases where we need to go through our node resolver to find something and it fails because resolve doesn't yet support exports

This PR removes the need for resolve in our node resolver 馃憤 I've marked this PR as breaking because it is possible that it could change behaviour 馃

@mansona mansona requested a review from ef4 November 28, 2023 20:58
@ef4 ef4 removed their request for review November 28, 2023 23:11
@ef4
Copy link
Contributor

ef4 commented Nov 28, 2023

The goal here is fine, the question is whether you can get all the tests to pass without the resolvableExtensions.

@patricklx
Copy link
Contributor

it could to both then?
the resolvableExtensions should only be a problem for the real app files.
all other package (addons) files should already be converted to .js files, right?

ef4 added a commit that referenced this pull request Dec 6, 2023
I found a case where we were accidentally leaking state between WebpackModuleRequests. This is hard to avoid since webpack *really* pushes us toward using mutation on the shared ResolveData object.

I solved it by making our mutations happen only at the precise moment when we're letting a request bubble back out to webpack.

This is expected to fail tests however because this bug was masking another bug. Specifically, we have several test apps that use things like the `page-title` helper. That helper is (1) used with ambiguous syntax, (2) published in a v2 addon. It turns out we cannot resolve the helper due to #1686, and this was only "working" by accident since the failed attempt at resolving the helper got leaked back to webpack and webpack *does* understand package.json exports and would find it.
I found a case where we were accidentally leaking state between WebpackModuleRequests. This is hard to avoid since webpack *really* pushes us toward using mutation on the shared ResolveData object.

I solved it by making our mutations happen only at the precise moment when we're letting a request bubble back out to webpack.

This is expected to fail tests however because this bug was masking another bug. Specifically, we have several test apps that use things like the `page-title` helper. That helper is (1) used with ambiguous syntax, (2) published in a v2 addon. It turns out we cannot resolve the helper due to #1686, and this was only "working" by accident since the failed attempt at resolving the helper got leaked back to webpack and webpack *does* understand package.json exports and would find it.
ef4 added a commit that referenced this pull request Dec 6, 2023
I found a case where we were accidentally leaking state between WebpackModuleRequests. This is hard to avoid since webpack *really* pushes us toward using mutation on the shared ResolveData object.

I solved it by making our mutations happen only at the precise moment when we're letting a request bubble back out to webpack.

This is expected to fail tests however because this bug was masking another bug. Specifically, we have several test apps that use things like the `page-title` helper. That helper is (1) used with ambiguous syntax, (2) published in a v2 addon. It turns out we cannot resolve the helper due to #1686, and this was only "working" by accident since the failed attempt at resolving the helper got leaked back to webpack and webpack *does* understand package.json exports and would find it.
@ef4
Copy link
Contributor

ef4 commented Dec 7, 2023

In 000d1ea I fixed a nasty bug, and it revealed other problems which probably can't be fixed without doing this PR, which is why I added the commit to here.

Then I was investigating the test failures and found a very annoying issue, related to the fact that pnpm extends NODE_PATH in bin shims, and require.resolve always checks NODE_PATH even when you override paths. So we're finding extra stuff by mistake, which leads to very confusing downstream failures.

@patricklx
Copy link
Contributor

patricklx commented Dec 7, 2023

Ah, i fixed the issues you have here: #1695
But maybe i also fixed the in a wrong way

8c9be4d
f675826
d53a80d

@ef4
Copy link
Contributor

ef4 commented Dec 7, 2023

Next issue here is that require.resolve('.', { paths }) doesn't actually do the same thing that normal require.resolve('.') does. We'll need a better strategy for self-imports.

Probably we can expand our notions of canResolveFrom so that for every addon we know a safe "outside" vantage point to resolve it from. For the app we can construct such a place if necessary, using symlinks.

@ef4
Copy link
Contributor

ef4 commented Dec 9, 2023

This is finally all passing!

I don't think this actually ended up breaking. I'm going to relabel it bugfix.

@ef4 ef4 added bug Something isn't working and removed breaking labels Dec 9, 2023
@ef4 ef4 changed the title use require.resolve for node resolving (to support exports) Resolver bugfixes Dec 9, 2023
@ef4 ef4 merged commit b33cb4c into main Dec 9, 2023
203 checks passed
@ef4 ef4 deleted the require-resolve branch December 9, 2023 02:08
@github-actions github-actions bot mentioned this pull request Dec 9, 2023
@patricklx patricklx mentioned this pull request Dec 9, 2023
7 tasks
@mansona
Copy link
Member Author

mansona commented Dec 13, 2023

@ef4 if this wasn't actually breaking how do you feel about backporting this to stable? 馃 this is required for v2 addons to enable type: module (if they want to)

@ef4
Copy link
Contributor

ef4 commented Dec 13, 2023

I'd want to hear from some large-ish apps that confirmed unstable is working for them. It's not unlikely that this does break somebody, given the wide range of legacy behaviors we're trying to support. So we have to weigh the risk of spending several days investigating bugs reports vs the benefit of accelerating getting these changes out.

This was referenced Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants