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(html): public asset urls always being treated as paths (fix #11857) #11870

Merged
merged 3 commits into from Mar 22, 2023

Conversation

Codex-
Copy link
Contributor

@Codex- Codex- commented Jan 31, 2023

Description

fix #11857

Adds a simple isUrl utility function that simply uses the node implementation of URL to validate a URL before attempting to normalize the value.

Additional context

Added tests, and I've tested this works against my issue repo with both a URL and non-url path, got the expected results.


What is the purpose of this pull request?

  • Bug fix

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

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title 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.

@Codex- Codex- changed the title fix(html) public asset urls always being treated as paths (fix #11857) fix(html): public asset urls always being treated as paths (fix #11857) Jan 31, 2023
@Codex-
Copy link
Contributor Author

Codex- commented Jan 31, 2023

Unsure why the macos tests timeout, I ran the tests successfully locally on macos 🤔

@patak-dev
Copy link
Member

Thanks for the PR @Codex-! The MacOS test was a flake, don't worry.
Would you add a test case for this one? https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md#extending-the-test-suite

@Codex-
Copy link
Contributor Author

Codex- commented Feb 2, 2023

@patak-dev really interesting test suite with playground, was interesting to dig into!

I've adapted the relative tests in assets to a new suite url, hopefully this makes sense to do.

I also validated the testing by stepping through with debugging to check we're asserting the path for a URL as expected, and explicitly so double slashes don't get broken accidentally

@Codex- Codex- force-pushed the public_asset_url_fix branch 2 times, most recently from 4836205 to af15cc6 Compare February 14, 2023 22:53
@Codex- Codex- force-pushed the public_asset_url_fix branch 2 times, most recently from 0f8e930 to bd97936 Compare March 15, 2023 21:12
@Codex-
Copy link
Contributor Author

Codex- commented Mar 15, 2023

  • Rebased
  • Updated added tests to match upstream move to esm

@patak-dev is there anything you want from me to get this moving? :)

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests! Sorry for the delay.
This looks good to me, we need another review now to merge it 👍🏼

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 16, 2023
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.

Thanks!

@patak-dev patak-dev merged commit 46d1352 into vitejs:main Mar 22, 2023
11 checks passed
@Codex- Codex- deleted the public_asset_url_fix branch March 22, 2023 11:09
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.

Using a full url as the base path generates bad parameters for @font-face url
3 participants