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

esbuild resolver for Vite dependency optimization #1614

Closed
wants to merge 3 commits into from

Conversation

lolmaus
Copy link
Collaborator

@lolmaus lolmaus commented Sep 27, 2023

Taking over @ef4's branch esbuild-resolver. Rebased it against main.

@lolmaus
Copy link
Collaborator Author

lolmaus commented Sep 27, 2023

Here's a demo app that uses it and it has an issue (see comments over the link):
mansona/ember-vite-app#1

@lolmaus lolmaus marked this pull request as ready for review October 2, 2023 10:30
@lolmaus
Copy link
Collaborator Author

lolmaus commented Oct 2, 2023

@mansona and me have found the reason why the build of ember-vite-app build was failing after using @ef4's EsbuildResolver.

Instead of having EsbuildResolver resolve external dependencies, we have have it mark as external: true and do nothing, which lets Vite take over and do the right thing via Embroider.

Notes:

  1. This may not be the most performance-efficient thing to do, but it seems to be at least correct, so we should consider merging this and iterate/improve on the implementation.
  2. There may be more dead code in @ef4's commit after we added the early return.

@@ -989,6 +989,10 @@ export class CompatAppBuilder {
this.addResolverConfig(resolverConfig);
let babelConfig = this.babelConfig(resolverConfig);
this.addBabelConfig(babelConfig);
writeFileSync(
Copy link
Collaborator

Choose a reason for hiding this comment

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

✂️ ?

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

I would feel better about this if we have some tests that used rollup build, but I understand there is a bit of a catch-22 here, and progress does need to be made.

@lolmaus lolmaus marked this pull request as draft October 11, 2023 14:26
@mansona
Copy link
Member

mansona commented Feb 1, 2024

This went a different direction: #1650

@mansona mansona closed this Feb 1, 2024
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.

None yet

4 participants