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

Prevent query-params added by vite to be passed to core logic #1846

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

BlueCutOfficial
Copy link
Collaborator

@BlueCutOfficial BlueCutOfficial commented Mar 20, 2024

Context
PR #1805 highlighted how Vite sometimes adds internal query parameters to requests. For instance, if you include a CSS file directly in index.html with a link tag pointing to a stylesheet, then Vite will internally add the query parameter ?direct to the request so it knows it has to return the stylesheet as is without wrapping it in some hot-reload-related JS. This is just one example of the sort of things Vite can do for its own reason.

The problem is that Embroider doesn't expect this kind of query params to exist, for instance when dealing with virtual files, and we can't let the core part of Embroider know about that: it is bundler-agnostic, it should behave the same way if you use Vite, or Webpack, or whatever.

Problem to solve
Everything that has to do with Vite query params should stay in packages/vite/src and from this point, we need to make sure that:

  • query-params added by Vite keep being propagated from request to request,
  • but are always removed from the specifier when we call the packages/core/src functions that are bundler-agnostic.

How to test

  • No regression when testing the vite-app
  • If the changes are used in PR Module resolver: virtualize vendor.css #1805, then we should be able to get rid of the last commit that handles isDirect in Vite resolver, and the vendor.css should still be generated correctly.

@BlueCutOfficial BlueCutOfficial force-pushed the handle-vite-query-params branch 3 times, most recently from 55def40 to 7b2ea00 Compare March 21, 2024 11:09
@BlueCutOfficial BlueCutOfficial marked this pull request as ready for review March 21, 2024 12:32
@BlueCutOfficial BlueCutOfficial self-assigned this Mar 21, 2024
@ef4 ef4 mentioned this pull request Mar 26, 2024
Comment on lines 33 to 34
let cleanSource = cleanUrlQueryParams(source);
let queryParams = getUrlQueryParams(source);
Copy link
Collaborator 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 replace these regexp-based methods with a URL object, but it caused trouble because the source can be a relative path. Here are examples of specifiers I get with both methods:

URL   : /app, 
Regexp: ../app, 

URL   : /-embroider-implicit-modules.js, 
Regexp: ./-embroider-implicit-modules.js, 

URL   : /packages/macros/src/addon/es-compat2, 
Regexp: ../../../../../../packages/macros/src/addon/es-compat2, 

…> when instantiating a new RollupModuleRequest from an import to resolve
…er URL over Regexp to clean-up when possible
Comment on lines 55 to 58
let { src, watches } = virtualContent(
cleanUrlQueryParams(id.slice(virtualPrefix.length)),
resolverLoader.resolver
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only parameter I have on load is the id, which corresponds - I guess - to a result.id as it contains the query params. I don't see any obvious way to avoid cleaning again the query params before passing to virtualContent.

@ef4 ef4 merged commit bdb7f3d into main Mar 28, 2024
93 checks passed
@ef4 ef4 deleted the handle-vite-query-params branch March 28, 2024 15:14
@mansona mansona added the enhancement New feature or request label Apr 4, 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
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants