-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Storage] port PR #24953 from storage/stable branch #26279
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the tests is good, not sure if we need all of the non-test changes.
@@ -268,7 +268,9 @@ export function appendToURLPath(url: string, name: string): string { | |||
path = path ? (path.endsWith("/") ? `${path}${name}` : `${path}/${name}`) : name; | |||
urlParsed.pathname = path; | |||
|
|||
return urlParsed.toString(); | |||
const normalizedUrl = new URL(urlParsed.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremymeng the original PR was working around behavior inside URLBuilder
, but since we're using URL
to parse things now, do we need these changes? I'm curious if we undo this change if the test still passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the tests are still passing. I undid the src changes. Thanks for pointing out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for porting these tests!
#24953
URLBuilder
, but switched toURL