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

[URL] Ensure that path-only URLs do not have their paths erased by th… #27720

Merged
merged 2 commits into from Jun 25, 2021

Conversation

karwa
Copy link
Contributor

@karwa karwa commented Feb 21, 2021

…e pathname setter.

See: whatwg/url#582

@karwa
Copy link
Contributor Author

karwa commented Feb 21, 2021

The first added test (special URLs) is just documenting the status-quo, since there didn't seem to be a test for what happens if you set an empty string.

The second one shows the new behaviour I propose on adding to the standard.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Confirmed that the third test added here fails without the change and passes with it, in jsdom/whatwg-url.

"href": "foo:/",
"pathname": "/"
}
},
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add a test where the new pathname value is non-empty but rootless, like "test"? A root should be automatically created in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also add a test setting a path on a URL without a host? My initial late-night implementation of this change would've failed that test. Like this:

  •    {
    
  •        "comment": "Non-special URLs can have their paths erased",
    
  •        "href": "foo:///some/path",
    
  •        "new_value": "",
    
  •        "expected": {
    
  •            "href": "foo://",
    
  •            "pathname": ""
    
  •        }
    
  •    },
    

annevk pushed a commit to whatwg/url that referenced this pull request Jun 17, 2021
@annevk
Copy link
Member

annevk commented Jun 17, 2021

Ah, I merged the specification PR but I missed the outstanding comment here. @karwa do you still want to add that additional test?

@annevk
Copy link
Member

annevk commented Jun 25, 2021

I added the additional requested tests, once someone reviewed them this should be good to go.

@annevk annevk merged commit 77d54aa into web-platform-tests:master Jun 25, 2021
@karwa
Copy link
Contributor Author

karwa commented Jul 6, 2021

Apologies, I missed this - thanks for adding the tests, @annevk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants