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

Resolver refactor #1627

Merged
merged 6 commits into from Oct 20, 2023
Merged

Resolver refactor #1627

merged 6 commits into from Oct 20, 2023

Conversation

mansona
Copy link
Member

@mansona mansona commented Oct 5, 2023

This PR started as an effort myself and @ef4 were pairing on to see if we can figure out how to teach vite and esbuild about our dependencies a bit better

It ended up being a nice refactor of the resolver so that we could remove some codepaths and simplify what the resolver was doing. It will also (hopefully) help esbuild understand what is going on a bit better, but I have split this PR so that it only contains the resolver refactor and the Vite/esbuild stuff will follow on in a new PR 👍

@mansona mansona force-pushed the resolver-refactor branch 2 times, most recently from 229a475 to b23c2c0 Compare October 5, 2023 15:04
@mansona mansona force-pushed the resolver-refactor branch 2 times, most recently from 63551fe to baf04ed Compare October 11, 2023 10:58
@ef4
Copy link
Contributor

ef4 commented Oct 12, 2023

In a31e2d6 we realized that the changes this PR makes to resolveWithinPackage are good when resolving between packages, but unhelpful when we're just adjusting self-imports inside a package. Those are not places where we want to make our imports look like inter-package dependencies to Vite, and by moving the requests out of their owning package we just complicate the rest of the module-resolver checks.

So in that commit we changed it so that self-imports continue to use relative paths as they do on main, and we only use the new superFakeTarget.js for inter-package imports.

With this change, it's possible that we no longer need this PR to move the app from node_modules/.embroider/rewritten-app to node_modules/.embroider/rewritten-app/node_modules/my-app-name, because I think it's really only the self-import case that applies to the app. We should investigate whether we can roll that part of the PR back and still see things working.

@lolmaus
Copy link
Collaborator

lolmaus commented Oct 12, 2023

Down to 7 failures, just three unique test cases left!

@mansona mansona changed the title WIP Resolver refactor Resolver refactor Oct 17, 2023
@mansona mansona marked this pull request as ready for review October 17, 2023 21:19
outputPath: resolve(
locateEmbroiderWorkingDir(this.compatApp.root),
'rewritten-app',
'node_modules',
Copy link
Contributor

Choose a reason for hiding this comment

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

This should get backed out of the PR. Since you didn't need the fake target for the app, we also don't need to put the app inside this structure.

This will mean a bunch of test changes in the PR also won't be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! and I pulled out a bit more that was specific to the app renaming (as you mentioned)

lolmaus and others added 3 commits October 19, 2023 11:54
This restores the original behavior we had on main for self-imports only because those don't need to be visible as dependencies for Vite and are simpler for us to handle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants