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 should stop appending ?import for data URLs #2658

Closed
6 tasks done
g-plane opened this issue Mar 23, 2021 · 11 comments · Fixed by #2703
Closed
6 tasks done

Vite should stop appending ?import for data URLs #2658

g-plane opened this issue Mar 23, 2021 · 11 comments · Fixed by #2703

Comments

@g-plane
Copy link
Contributor

g-plane commented Mar 23, 2021

Describe the bug

Vite injects ?import query to data URL in dynamic import calls, which should be unexpected.

Reproduction

const blob = new Blob([/* */] { type: '...' })
const url = URL.createObjectURL(blob)

;(async () => {
  await import(/* @vite-ignore */ url) // <== Vite injects `?import` query here.
})()

I hope if we add @vite-ignore magic comment, Vite won't inject ?import query, otherwise it will break the URL and cause failing to load such resources.

The code of Vite below will mark "needs inject query": https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/importAnalysis.ts#L412

Then, Vite will inject at runtime: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/importAnalysis.ts#L413

I hope we can add a detection:

if (!hasViteIgnore) {
  needQueryInjectHelper = true
  str().overwrite(start, end, `__vite__injectQuery(${url}, 'import')`)
}

Edit: My another solution is to skip adding queries for URLs which start with blob: and data: at both compile-time and runtime.

System Info

Output of npx envinfo --system --npmPackages vite,@vitejs/plugin-vue --binaries --browsers:

  System:
    OS: Linux 5.11 Arch Linux
    CPU: (8) x64 AMD Ryzen 5 2500U with Radeon Vega Mobile Gfx
    Memory: 1.85 GB / 6.75 GB
    Container: Yes
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 15.12.0 - /usr/bin/node
    Yarn: 1.22.10 - /usr/bin/yarn
    npm: 7.6.3 - /usr/bin/npm
  Browsers:
    Firefox: 86.0.1
  npmPackages:
    @vitejs/plugin-vue: ^1.1.5 => 1.1.5 
    vite: ^2.1.0 => 2.1.2

Used package manager: npm

Logs


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

  • Read the Contributing Guidelines.
  • Read the docs.
  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • Provide a description in this issue that describes the bug.
  • Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
  • Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
@Shinigami92
Copy link
Member

thought (dx):

Magic comments are having the problem that they get removed if using compilerOptions.removeComments: true in tsconfig.json

Could it be possible to wrap the async import in a vite utility function?

e.g.:

import { viteImport } from 'vite'

const blob = new Blob([/* */] { type: '...' })
const url = URL.createObjectURL(blob)

;(async () => {
  await viteImport(url, { noImport: true }) // <== Vite injects `?import` query here.
})()
export async function viteImport(url, options) {
  // do things with url and options
  const imported = await import(url)
  // do things with imported and options
  return imported
}

@g-plane
Copy link
Contributor Author

g-plane commented Mar 23, 2021

Or, we can add a detection that checks if it starts with data: or blob:. If it's, skip adding ?import and any other queries.

@g-plane
Copy link
Contributor Author

g-plane commented Mar 24, 2021

@Shinigami92 That solution (adding viteImport) will make userland code harder to read, and the userland code will become less common, since it will be Vite-specific.

I'd like to submit a PR to add a detection that checks if URL starts with data: or blob: or not, but I'm not sure whether you accept this solution or not.

@patak-dev
Copy link
Member

Would you create a minimal repro? There may be a bug here.

The use of query strings is working well so far, and it is better aligned with import assertions. Other tools like WMR are considering to move away from prefixes.

@g-plane
Copy link
Contributor Author

g-plane commented Mar 24, 2021

Minimal reproduction repository: https://github.com/rosaric/vite-bug-repro .

@Shinigami92
Copy link
Member

@Shinigami92 That solution (adding viteImport) will make userland code harder to read, and the userland code will become less common, since it will be Vite-specified.

I'd like to submit a PR to add a detection that checks if URL starts with data: or blob: or not, but I'm not sure whether you accept this solution or not.

@g-plane Totally with you! That was just a proposal for an alternative for the /* @vite-ignore */-magic comment.
And this would/is already a vite specific solution.

@g-plane
Copy link
Contributor Author

g-plane commented Mar 24, 2021

I don't mind magic comments, because they're trivial and don't affect my code directly, but using viteImport would be a dirty way, which isn't elegant.

@Shinigami92
Copy link
Member

@g-plane But did you know that when using compilerOptions.removeComments: true they get removed and that way don't work?
I had this problem in webpack with webpackChunkName. Don't know if Vite maybe read them early enough, but if so we can create a new issue for that.

@g-plane
Copy link
Contributor Author

g-plane commented Mar 24, 2021

Yes, and my suggestion is to skip adding queries for blob: and data:, and this solution doesn't rely on magic comments.

@g-plane
Copy link
Contributor Author

g-plane commented Mar 26, 2021

Can I attempt to open a PR that adds a processing for URLs start with data: or blob:?

@Shinigami92
Copy link
Member

@g-plane You are always welcome to open any PR 😁
We are then happy to review and triage it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants