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

Refactor the resolve function to be the only public api to module-resolver #1794

Merged
merged 21 commits into from
Feb 29, 2024

Conversation

mansona
Copy link
Member

@mansona mansona commented Feb 8, 2024

No description provided.

@void-mAlex
Copy link
Collaborator

any chance this refactor could land on stable first? I'd like to have one api to call from the code mod and not support maybe Promises maybe sync style nodeResolve

@mansona
Copy link
Member Author

mansona commented Feb 15, 2024

any chance this refactor could land on stable first?

To be honest, that's quite unlikely 😞 This is in the class of things where it could potentially break things but has very little gain to people who are not using Vite, and is the main reason why we forked stable from main.

I'd like to have one api to call from the code mod

I mean... I don't see any reason why you need to use stable for the codemod 🤔 why not use unstable?

@void-mAlex
Copy link
Collaborator

I mean... I don't see any reason why you need to use stable for the codemod 🤔 why not use unstable?

because I want to use the codemod and I use stable :)

also it's more reasonable to give it to people who can't get to main and this could be the push/unblocker to get them there

@ef4
Copy link
Contributor

ef4 commented Feb 23, 2024

This is getting close. The remaining failure is in the vite-specific test, because as of this change we use the real esbuild and vite resolvers instead of the node resolver when locating components, and that is causing some trouble.

  • vite has a plugin during depscan that turns "not found" into "external" so it will be silently ignored by esbuild. But this is bad for us because when we're checking "does this file exist?" we want honest answers, rather than always getting "yes (but external)". It's probably OK to always treat external as not_found, at least during depscan.
  • we're also running into the dreaded Packages using self-referencing imports will fail to resolve ("moduleResolution": "node16" and package.json "exports") vitejs/vite#9731 because now component resolution uses the real vite resolver and the vite resolver is wrong for self-references in v2 addons (v1 addons get adjusted automatically by embroider to handle self-references, v2 addons are able to use the full package.json exports API and are supposed to be able to rely on it)

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Feb 23, 2024

nevermind

v2 addons are able to use the full package.json exports API and are supposed to be able to rely on it)

in my testing, this has only been true when type: module
demo here:

demo

Maybe i've been confusing actual modules with not-quite-modules (js with no type=module)

at least now we have a demo proving that self-imports are supposed to work 😅

@ef4
Copy link
Contributor

ef4 commented Feb 27, 2024

The remaining failure here seems to be because our esbuild external-to-not-found workaround is too broad. It's causing a dep like qunit to be reported as not_found, when I think vite is telling esbuild that's it's external during depscan.

@patricklx

This comment was marked as outdated.

@mansona mansona merged commit c52ad5c into main Feb 29, 2024
93 checks passed
@mansona mansona deleted the default-resolve branch February 29, 2024 13:08
@github-actions github-actions bot mentioned this pull request Feb 29, 2024
@github-actions github-actions bot mentioned this pull request Apr 4, 2024
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
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

5 participants