Skip to content

v7.x: backport a few WHATWG URL changes - #11069

Merged
TimothyGu merged 3 commits into
nodejs:v7.x-stagingfrom
TimothyGu:v7.x-staging
Jan 31, 2017
Merged

v7.x: backport a few WHATWG URL changes#11069
TimothyGu merged 3 commits into
nodejs:v7.x-stagingfrom
TimothyGu:v7.x-staging

Conversation

@TimothyGu

@TimothyGuTimothyGu commented Jan 30, 2017

Copy link
Copy Markdown
Member

These commits are from #10955 and #10906 unchanged. Since the WHATWG URL API is still in Stage: 1 Experimental, the removal of url.originFor() and of inspect() should not be a problem.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@nodejs-github-botnodejs-github-bot added dont-land-on-v4.x url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 30, 2017
@mscdexmscdex removed the url Issues and PRs related to the legacy built-in url module. label Jan 30, 2017
@targos

targos commented Jan 30, 2017

Copy link
Copy Markdown
Member

LGTM

@TimothyGu

TimothyGu commented Jan 30, 2017

Copy link
Copy Markdown
MemberAuthor

CI: https://ci.nodejs.org/job/node-test-pull-request/6108/

@joyeecheung

joyeecheung commented Jan 30, 2017•
edited
Loading

Copy link
Copy Markdown
Member

@TimothyGu Have you rebased and force pushed? There are a lot of previous commits in this PR now.

EDIT: or is this the CI's doing?

@TimothyGu

TimothyGu commented Jan 30, 2017

Copy link
Copy Markdown
MemberAuthor

@joyeecheung I believe someone force-pushed to v7.x-staging which led to this problem. Indeed, IRC #node-dev shows:

09:23:25 -- | Notice(nodejs-gh): [node] italoacasas force-pushed v7.x-staging
from 376b9a4 to 506a50b: https://git.io/vizvQ

@italoacasas

italoacasas commented Jan 30, 2017

Copy link
Copy Markdown

Sorry that this happens @TimothyGu, I had the need to drop a commit from staging.

@italoacasas

italoacasas commented Jan 30, 2017

Copy link
Copy Markdown

btw my understanding of backporting(maybe I'm wrong) is that you do a backport when the PR doesn't land clearly in the staging branch.

@TimothyGu

TimothyGu commented Jan 30, 2017

Copy link
Copy Markdown
MemberAuthor

@italoacasas, sorry I'm not super familiar with this topic. So if there are no conflicts and the PR is not labelled dont-land-on-v7.x, landing of these commits is automatic?

@italoacasas

italoacasas commented Jan 30, 2017

Copy link
Copy Markdown

@TimothyGu, unfortunately, is not automatic, normally someone from the release team should cherry-pick the commit to the staging branch.

@TimothyGu
url: stop exporting originFor()
57e5315
PR-URL: nodejs#10955Fixes: nodejs#10800
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@TimothyGu
url: do not public expose inspect methods on URL
499e75a
PR-URL: nodejs#10906
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@TimothyGu
url: define @@toStringTag as a data property
b94eec1
Even though this is not fully Web IDL spec-compliant, it is arguably the
best we can do. Following the spec would mean non-trivial performance
deterioration (10% when parsing a medium-length URL), while the current
getter behavior is not adopted by any implementer, and it causes some
spec ambiguity when the getter is called with !(this instanceof URL).
This commit adopts Chrome's behavior, and is consistent with
ECMAScript-defined classes while providing reasonable behaviors for
corner cases as well. Until the Web IDL spec is changed one way or
another, this is the way to go.
PR-URL: nodejs#10906
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@TimothyGu
TimothyGu merged commit b94eec1 into nodejs:v7.x-stagingJan 31, 2017
@TimothyGu

TimothyGu commented Jan 31, 2017

Copy link
Copy Markdown
MemberAuthor

Either way, since I've already taken the time to make this PR, I'll just land it myself.

Done in 57e5315, 499e75a, and b94eec1.

@TimothyGu
TimothyGu deleted the v7.x-staging branch January 31, 2017 00:55
@joyeecheung

joyeecheung commented Jan 31, 2017

Copy link
Copy Markdown
Member

So if I understand correctly, no backport has been requested from @nodejs/release to @TimothyGu (and this applies cleanly)? Anyway I think the release team should be notified, just to be safe.

This was referenced Mar 7, 2020
Open
Open
Open
Open
Open
@alisccoaliscco mentioned this pull request Dec 1, 2023
Open
@leonardoadameleonardoadame mentioned this pull request Dec 1, 2023
Open
This was referenced Dec 1, 2023
Open
Open
This was referenced Dec 1, 2023
Open
Open
This was referenced Dec 1, 2023
Open
Open
@adamlaskaadamlaska mentioned this pull request Dec 2, 2023
Open
@alisccoaliscco mentioned this pull request Dec 2, 2023
Open
@leonardoadameleonardoadame mentioned this pull request Dec 2, 2023
Open
This was referenced Dec 2, 2023
Open
Open
Open
Open
@leonardoadameleonardoadame mentioned this pull request Dec 2, 2023
Open
This was referenced Dec 2, 2023
Open
Open
Open
This was referenced Dec 2, 2023
Open
Open
This was referenced Dec 2, 2023
Open
Open
This was referenced Dec 3, 2023
Open
Open
Open
@alisccoaliscco mentioned this pull request Dec 3, 2023
Open
@leonardoadameleonardoadame mentioned this pull request Dec 3, 2023
Open
@alisccoaliscco mentioned this pull request Dec 3, 2023
Open
@leonardoadameleonardoadame mentioned this pull request Dec 5, 2023
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheungjoyeecheungjoyeecheung approved these changes
+1 more reviewer
@italoacasasitaloacasasitaloacasas approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

whatwg-urlIssues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@TimothyGu@targos@joyeecheung@italoacasas@mscdex@nodejs-github-bot