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

vite deps: correctly handle addon templates and addon app files 2 #2078

Closed
wants to merge 1 commit into from

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Aug 24, 2024

  • makes external templates ignored
  • optimised templates do not get a pairing
  • hbs extension search during bundle
  • externalize app files during bundling
    • since the request comes from a dep file we cannot know the original addon file. Which is why the from file will be encoded into the specifier.
    • This is required because the resolution can change during development
  • stage2 tests now correctly uses optmized deps

@patricklx patricklx changed the title fix app deps fix stage2 app deps Aug 24, 2024
@patricklx patricklx changed the title fix stage2 app deps fix stage2 scenario app deps Aug 24, 2024
@patricklx
Copy link
Contributor Author

patricklx commented Aug 25, 2024

@ef4 this fixes some esbuild issues .
It sometimes fails on windows though

@patricklx patricklx changed the title fix stage2 scenario app deps fix dep optimiser for templates Aug 26, 2024
@patricklx patricklx changed the title fix dep optimiser for templates fix dep optimiser for addon templates Aug 26, 2024
embroider: {
enableCustomResolver: false,
meta: request.meta,
const extensions = ['', '.hbs'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ef4 I tried to move the extension resolving here to defaultResolve, but it does not work.
I think the issue is that when we resolve files in addons, we do use a relative specifier with a from file in the rewritten-addons. What esbuild then sees is something like

{
   type: 'ignored',
   result: {
    external: true,
    path: './components/hello-world'
  }
}

so it has relative path as the externalized ID, I think this will confuse the system between esbuild and vite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did the same changes in #1876 and there it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could do this as an intermediate step? I would change the tests to do the checks at runtime

@patricklx patricklx changed the title fix dep optimiser for addon templates do not optimize addon app files Sep 5, 2024
@patricklx patricklx changed the title do not optimize addon app files improve dep optimizer Sep 5, 2024
@patricklx patricklx changed the title improve dep optimizer correctly handle addon templates and addon app files Sep 5, 2024
@patricklx patricklx changed the title correctly handle addon templates and addon app files vite deps: correctly handle addon templates and addon app files Sep 5, 2024
@ef4
Copy link
Contributor

ef4 commented Sep 6, 2024

Thanks for working on the stage2 tests here, I actually tried to use your version in #2093 but realized there was just so much irrelevant stuff lurking in that test suite that it needed rewriting anyway, and I was able to drop a lot of it.

@ef4
Copy link
Contributor

ef4 commented Sep 10, 2024

I would suggest merging master and adding tests to the vite-internals tests, which I have now expanded so they run against vite dev and see the effects of the dep optimizer.

@patricklx
Copy link
Contributor Author

this looks like its fixed, closing

@patricklx patricklx closed this Sep 13, 2024
@patricklx patricklx changed the title vite deps: correctly handle addon templates and addon app files vite deps: correctly handle addon templates and addon app files 2 Sep 18, 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.

3 participants