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): ignore rewrite external urls #14774

Merged
merged 2 commits into from
Oct 27, 2023
Merged

fix(html): ignore rewrite external urls #14774

merged 2 commits into from
Oct 27, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 27, 2023

Description

Fix #14770

Make sure relative urls that don't start with a . are not external urls by checking :. I think that should be good enough 🤔

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Oct 27, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy bluwy added this to the 5.0 milestone Oct 27, 2023
@patak-dev
Copy link
Member

Maybe we should use isExternalUrl and isDataUrl here? If checking for : is enough, we could replace these by checking but I'm not sure about that.
Related PR: #14744

@bluwy
Copy link
Member Author

bluwy commented Oct 27, 2023

Oh I didn't realize #14744 was fixing a Vite beta regression (or it already exists before?)

I didn't use those as I figured a : is more performant and I can't think of cases where : exist and we want to transform it. I guess the other parts of the codebase is isExternalUrl and isDataUrl so it doesn't incorrectly match Windows paths, but in this HTML case we also wouldn't want to match Windows paths.

If you prefer the consistency, I don't mind changing this to isExternalUrl and isDataUrl though (and also the #). It looks like #14744 isn't completed yet.

@patak-dev
Copy link
Member

Yes, for #14744, I think it is better to use the functions directly instead of extracting the excludeUrl util. I'll get to it if it doesn't progress.

About using isExternalUrl/isDataUrl, it seems to me there are other places that are the same as this one where it is being used. I'm ok if you'd like to merge this one with the perf optimization already (and maybe later we could review others). It would be good to have a comment explaining that : discards external and data URLs. I wonder why we are checking only for https:// in isExternal though, your will also hit other protocols like ipfs://, that we should also probably be ignoring as external, no?

@bluwy
Copy link
Member Author

bluwy commented Oct 27, 2023

I can add the comment 👍 Yeah the external issue seems to be related to #13540. Quite a bit of refactors if we want to fix this up and definitely worth reviewing again.

@bluwy bluwy merged commit d6d1ef1 into main Oct 27, 2023
9 checks passed
@bluwy bluwy deleted the html-external-rewrite branch October 27, 2023 09:09
@patak-dev patak-dev mentioned this pull request Nov 4, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[5.0 beta] http:// urls in index.html are rewritten on non-index pages
2 participants