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

fix(client): don't inject queries for data URLs (fixes #2658) #2703

Merged
merged 1 commit into from
Mar 29, 2021
Merged

fix(client): don't inject queries for data URLs (fixes #2658) #2703

merged 1 commit into from
Mar 29, 2021

Conversation

g-plane
Copy link
Contributor

@g-plane g-plane commented Mar 26, 2021

Description

Make it not inject queries for data URLs.

Fixes #2658

Additional context

Appending queries to data URLs (those URLs that start with data: or blob:) will break the data because data is encoded as string. This PR adds a detection for data URLs, and it will return those URLs as-is.

Is this direction right?


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 26, 2021
@g-plane
Copy link
Contributor Author

g-plane commented Mar 26, 2021

I don't know why the tests would fail.

@Shinigami92 Shinigami92 added the help wanted Extra attention is needed label Mar 27, 2021
@g-plane
Copy link
Contributor Author

g-plane commented Mar 27, 2021

@Shinigami92 All tests passed now.

@Shinigami92 Shinigami92 added 🔍 review needed and removed help wanted Extra attention is needed labels Mar 27, 2021
@g-plane
Copy link
Contributor Author

g-plane commented Mar 29, 2021

@patak-js Would you like to review this PR?

@patak-dev
Copy link
Member

We'll need @yyx990803 input here, as I don't fully understand if this change will end up affecting other parts of the system.

But I think that we shouldn't be bailing out inside of injectQuery because if you called that function, it means that you need that query to be injected in that URL. So IMO we should do this check out of injectQuery to be explicit about this

@yyx990803
Copy link
Member

The only place the client-side injectQuery is used is by the code injected by importAnalysis here, where the URL may be fully dynamic, so we can't reliably skip the injection via static analysis. Doing the check on client makes sense.

@patak-dev patak-dev merged commit 86753d6 into vitejs:main Mar 29, 2021
@g-plane g-plane deleted the issue-2658 branch March 30, 2021 01:17
@DenizUgur
Copy link

Sorry to bring up an old PR but it seems like this change has been reverted. Any reason why this has been done? Or is there an another way of disabling the injectQuery import?

@patak-dev
Copy link
Member

@DenizUgur please create an issue with a minimal reproduction and more context of your problem, (you can link to this PR as additional info) so we can properly track your report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite should stop appending ?import for data URLs
5 participants