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: when the file path is an absolute path, parsing causes parameter loss #10449

Merged
merged 3 commits into from Oct 25, 2022

Conversation

yzydeveloper
Copy link
Contributor

@yzydeveloper yzydeveloper commented Oct 12, 2022

Description

Fixes #10448

When the file path is an absolute path

For example E:/_yzydeveloper/vite-resolve-url-encode/node_modules/@ui/components/Test.vue?vue&type=template&lang.js

new URL will resolve to

URL {
  href: 'e:/_yzydeveloper/vite-resolve-url-encode/node_modules/@ui/components/Test.vue?vue&type=template&lang.js',
  origin: 'null',
  protocol: 'e:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: '/_yzydeveloper/vite-resolve-url-encode/node_modules/@ui/components/Test.vue',
  search: '?vue&type=template&lang.js',
  searchParams: URLSearchParams { 'vue' => '', 'type' => 'template', 'lang.js' => '' },
  hash: ''
}

The protocol at this time is e:,caused pathToFileURL processing error,cause parameter loss.

error handling

export function injectQuery(url: string, queryToInject: string): string {
  // ...
  if (resolvedUrl.protocol !== 'relative:') {
    resolvedUrl = pathToFileURL(url)
  }
  // ...
}

normal handling

export function injectQuery(url: string, queryToInject: string): string {
  // ...
  if (resolvedUrl.protocol === 'file:') {
    resolvedUrl = pathToFileURL(url)
  }
  // ...
}

Additional context


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.

@yzydeveloper yzydeveloper changed the title fix: When fileUrl is an absolute path, the path parameter will be cle… fix: When the file path is an absolute path, parsing causes parameter loss Oct 12, 2022
@sapphi-red
Copy link
Member

Would you add a test here?

describe('injectQuery', () => {

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 20, 2022
@yzydeveloper
Copy link
Contributor Author

@sapphi-red f7c41be Test added

Comment on lines 326 to 328
if (resolvedUrl.protocol === 'file:') {
resolvedUrl = pathToFileURL(url)
}
Copy link
Member

Choose a reason for hiding this comment

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

Because we are not getting path part from the parsed URL, I guess we can remove this condition. refs #2435

Also this condition now doesn't make sense.
When resolvedUrl.protocol is file:, url has file: protocol. Then, we are passing file URL to pathToFileURL. This won't work.

/cc @poyoho

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 agree that this condition should be removed.

Copy link
Member

Choose a reason for hiding this comment

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

@yzydeveloper Would you remove this condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sapphi-red This condition has been removed. 5a42330

@sapphi-red sapphi-red added the regression The issue only appears after a new release label Oct 23, 2022
@sapphi-red sapphi-red linked an issue Oct 23, 2022 that may be closed by this pull request
7 tasks
@sapphi-red
Copy link
Member

I confirmed this PR fixes #10278.

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thank you!

@sapphi-red sapphi-red changed the title fix: When the file path is an absolute path, parsing causes parameter loss fix: when the file path is an absolute path, parsing causes parameter loss Oct 23, 2022
@patak-dev patak-dev merged commit df86990 into vitejs:main Oct 25, 2022
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) regression The issue only appears after a new release
Projects
None yet
3 participants