Skip to content

fix: when the file path is an absolute path, parsing causes parameter loss#10449

Merged
patak-cat merged 3 commits intovitejs:mainfrom
yzydeveloper:fix-resolve-url
Oct 25, 2022
Merged

fix: when the file path is an absolute path, parsing causes parameter loss#10449
patak-cat merged 3 commits intovitejs:mainfrom
yzydeveloper:fix-resolve-url

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
@sapphi-red sapphi-red requested review from bluwy and patak-cat October 25, 2022 05:14
@patak-cat patak-cat 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

Development

Successfully merging this pull request may close these issues.

When the file path is an absolute path, injectQuery will clear the parameters. Component import fails if template html is referenced using src

3 participants