breaking: fix path resolution#11276
Conversation
🦋 Changeset detectedLatest commit: 70f0e20 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dummdidumm
left a comment
dummdidumm
left a comment
There was a problem hiding this comment.
What were the implications of this buggy behavior? Was it resulting in a bug in some way?
| if (path[0] === '#') return base + path; | ||
| if (path === '') return base; | ||
| // special case | ||
| if (path[0] === '/' && path[1] === '/') return path; |
There was a problem hiding this comment.
what's this special case about? Is this some extra thing we need to mention in the docs? What does this mean for ''?
There was a problem hiding this comment.
protocol-relative URLs should remain protocol-relative, but the new URL(...) technique means we lose that information. so we need to special-case it
| "/test♥hex" | ||
| "/test%22quotation", | ||
| "/test%E2%99%A1decimal", | ||
| "/test%E2%99%A5hex" |
There was a problem hiding this comment.
The previous results had the character references persisted - will this somehow affect other stuff negatively / in a breaking way now that they no longer are?
There was a problem hiding this comment.
I figured if it did, tests would start failing. Seems fine. Though we're probably now double-decoding at prerender time
There was a problem hiding this comment.
Oof. Double-decoding is bad and would result in breakages. Maybe we don't have a test for prerendering an encoded URL?
There was a problem hiding this comment.
We do:
If there's a bug, it's very well-hidden, and we can fix it when it reveals itself. It shouldn't block merging
Yes #11273 (comment) |
|
I mean, yes, this is a bug that showed itself there, but I don't understand yet what actual issue this would cause. Maybe the current behavior is actually more desireable? |
|
It doesn't matter if it's more desirable, it's incorrect! |
|
That's like saying measuring in inches is incorrect (obviously is, wtf America) and using centimeters is the better variant, but if it turns out that it works in practise and noone has an issue with it, why change it? |
|
Because it's stupidly confusing if we resolve URLs differently to everyone else on the planet. The fact that it makes it marginally easier to do something you almost certainly don't want to do (emulating confusing default cookie path behaviour) is a terrible reason to keep the broken behaviour |
|
Ok fine. I just really wished we would've found some special string to signal "I want the default browser behavior" 😄 |
3 participants
Our
resolveutility was returning incorrect results in the case wherepathwas just a.character, leading to confusion such as #11273 (comment).This implementation delegates to
new URL(...), so it's more or less guaranteed to be correct whatever edge cases we throw at itPlease don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.