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: ?import with trailing = added by some servers #3805

Merged
merged 3 commits into from
Jun 16, 2021
Merged

Conversation

t3dotgg
Copy link
Contributor

@t3dotgg t3dotgg commented Jun 15, 2021

Description

Vercel's cli tries to resolve all paths before passing them to the frontend dev server. During these checks, it parses query strings. During this parse, it modifies ?import to be ?import=, which the vite dev server fails to resolve :(

To prevent this from breaking in Vercel and other dev servers, I've modified the ?import query param to include a value: ?import=vitedev

Most servers and URL parsers assume that querystrings are key:value pairs, bound with =, and I think it is reasonable to encode that assumption. At the very least, a trailing = should be supported (as most parsers assume this is the case).

That said, I'm more than happy to investigate another solution - let me know if you would prefer an alternative :)

Additional context

Issue:
#3781

Attempt to fix on Vercel CLI side, includes good discussion on why this should be fixed here:
vercel/vercel#6359

Example of = being appended by standard browser URL interface

> u = new URL('http://a.com/p?import')

// good
> u.href
'http://a.com/p?import'

// modify
> u.searchParams.set('a', 'b')

// bad
> u.href
'http://a.com/p?import=&a=b'

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.

@t3dotgg
Copy link
Contributor Author

t3dotgg commented Jun 15, 2021

More context:

Seems like most URL parsers expect the = to be included, and add it when it is not. Both the node url package and the browser new URL interface will append the =
vercel/vercel#6359 (comment)

@t3dotgg t3dotgg changed the title Fix ?import querystring breaking in other dev servers (Vercel support) Fix ?import querystring breaking in other servers and URL parsers (including Vercel CLI) Jun 15, 2021
@t3dotgg
Copy link
Contributor Author

t3dotgg commented Jun 15, 2021

Caught one import string update I missed for a CSS resolver, tests should pass now :)

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jun 15, 2021
@t3dotgg
Copy link
Contributor Author

t3dotgg commented Jun 15, 2021

Just switched from ?import=vitedev to ?import=, worked against my tests locally :) Let me know if any other changes should be made @patak-js

packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved
@t3dotgg
Copy link
Contributor Author

t3dotgg commented Jun 15, 2021

Going to update regex momentarily. Wanted to share some thoughts for vite's query params in the longer term

I do not plan on making these changes, and knowing the ecosystem expectations around these flags, they would be breaking and as such best suited for a major release

IMO, there is value in changing these to better match the expected field=value query param paradigm that most implementations are assuming. I agree that it will make the URL more noisy, but I think the explicit indication of "where these query params are coming from" is beneficial.

Perhaps a vite-specific key with many values would be better? I.e. ?viteflags=import,worker,inline

@t3dotgg
Copy link
Contributor Author

t3dotgg commented Jun 15, 2021

Should be good for another look @patak-js

@patak-dev
Copy link
Member

Perhaps a vite-specific key with many values would be better? I.e. ?viteflags=import,worker,inline

The issue with this is that this form

import Worker from './shader.js?worker&inline'

is a lot terser and has a better signal to noise ratio than

import Worker from './shader.js?viteflags=worker,inline'

But the standard will be quite verbose here also

import json from "./foo.json" assert { type: "json" };

So maybe we could use something like ?assert=worker,inline? But I don't think this would be changed though if there is not a use case that is failing with the current one (that the ecosystem has already adopted)

@patak-dev patak-dev changed the title Fix ?import querystring breaking in other servers and URL parsers (including Vercel CLI) fix: ?import with trailing = added by some servers Jun 16, 2021
@patak-dev patak-dev merged commit 460d1cd into vitejs:main Jun 16, 2021
javastation pushed a commit to javastation/vite that referenced this pull request Jun 22, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Jun 25, 2021
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.

None yet

3 participants