Skip to content

core(artifacts): encapsulate node details in an object [reverted] #11474

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.

Sign up for GitHub

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

Jump to bottom
Merged
paulirish merged 8 commits into from
Nov 20, 2020
Merged

core(artifacts): encapsulate node details in an object [reverted] #11474

paulirish merged 8 commits into from
Nov 20, 2020

Conversation

@adrianaixba
Copy link
Collaborator

@adrianaixba adrianaixba commented Sep 22, 2020 •
edited
Loading

follow up to #11405

breaking change, creating an object for node details

fixes #9947

@adrianaixba adrianaixba requested a review from a team as a code owner September 22, 2020 17:09
@adrianaixba adrianaixba requested review from paulirish and removed request for a team September 22, 2020 17:09
@patrickhulce patrickhulce mentioned this pull request Sep 22, 2020
Closed
9 tasks
@connorjclark
Copy link
Collaborator

connorjclark commented Sep 22, 2020

Which part is breaking, taking into account our "PublicArtifacts"? I think it's just one or two artifacts... could we have both the object and the toplevel properties for now, and mark an item in #11207 to remove the old way?

adrianaixba
adrianaixba commented Sep 22, 2020
View reviewed changes
@connorjclark connorjclark changed the title core: add node details object core(artifacts): encapsulate node details in an object Sep 22, 2020
@adrianaixba
Copy link
Collaborator Author

adrianaixba commented Sep 22, 2020

Which part is breaking, taking into account our "PublicArtifacts"? I think it's just one or two artifacts... could we have both the object and the toplevel properties for now, and mark an item in #11207 to remove the old way?

@connorjclark Based on the PublicGathererArtifacts, it would be about 5 that use the changed artifacts: ImageElements, LinkElements, ScriptElements, IFrameElements, and FormElements. Is that what you were referring to?

@connorjclark
Copy link
Collaborator

connorjclark commented Sep 22, 2020 •
edited
Loading

Which part is breaking, taking into account our "PublicArtifacts"? I think it's just one or two artifacts... could we have both the object and the toplevel properties for now, and mark an item in #11207 to remove the old way?

@connorjclark Based on the PublicGathererArtifacts, it would be about 5 that use the changed artifacts: ImageElements, LinkElements, ScriptElements, IFrameElements, and FormElements. Is that what you were referring to?

Yes. more than I thought

Alternatively, we can hold this entire PR until December (v7).

... FormElements

Yikes, that was a mistake, I'm going to revert this.

@brendankenny brendankenny mentioned this pull request Sep 29, 2020
Closed
@connorjclark
Copy link
Collaborator

connorjclark commented Oct 6, 2020

@adrianaixba, do you want to modify this PR to not be breaking so it can land now, or should we close this PR and mark #9947 as a v7 item?

@adrianaixba
Copy link
Collaborator Author

adrianaixba commented Oct 6, 2020

@connorjclark marking it as v7 sounds good to me

@adrianaixba adrianaixba closed this Oct 6, 2020
@connorjclark connorjclark mentioned this pull request Oct 13, 2020
Closed
@paulirish paulirish added the 7.0 label Nov 9, 2020
@paulirish paulirish reopened this Nov 9, 2020
paulirish
paulirish requested changes Nov 16, 2020
View reviewed changes
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

comprehensive job!! nice

since putting this up, the new preload-lcp-image audit landed.. and it needs some minor node. additions

and a few more comments below

paulirish
paulirish approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

2 nits. lgtm otherwise

nice job on this!

@paulirish paulirish merged commit 19160c2 into master Nov 20, 2020
@paulirish paulirish deleted the nodedetails-object branch November 20, 2020 22:05
@paulirish
Copy link
Member

paulirish commented Nov 20, 2020

🎈 🎈 🎈 🎈 🎈 🎈 🎈 🎈 🎈 🎈 🎈

paulirish added a commit that referenced this pull request Nov 20, 2020
@paulirish
Revert "core(artifacts): encapsulate node details in an object (#11474)"
666057a 
This reverts commit 19160c2.
@paulirish paulirish mentioned this pull request Nov 20, 2020
Merged
paulirish added a commit that referenced this pull request Nov 20, 2020
@paulirish
Revert "Revert "core(artifacts): encapsulate node details in an object (
2470421 
#11474)""

This reverts commit 666057a.
@paulirish paulirish mentioned this pull request Nov 20, 2020
Merged
devtools-bot pushed a commit that referenced this pull request Nov 21, 2020
@paulirish
misc(revert): core(artifacts): encapsulate node details in an object (#…
bb795cb 
…11474) (#11694)
@paulirish paulirish changed the title core(artifacts): encapsulate node details in an object core(artifacts): encapsulate node details in an object [reverted] Nov 30, 2020
paulirish added a commit that referenced this pull request Dec 7, 2020
@paulirish
Revert "Revert "core(artifacts): encapsulate node details in an object (
8e28eaf 
#11474)""

This reverts commit 666057a.
@paulirish paulirish restored the nodedetails-object branch December 7, 2020 23:59
patrickhulce pushed a commit that referenced this pull request Dec 8, 2020
@paulirish
core(artifacts): encapsulate node details in an object (#11695, reland …
2cbbd28 
…#11474)
@brendankenny brendankenny deleted the nodedetails-object branch December 16, 2020 02:03
@connorjclark connorjclark mentioned this pull request Feb 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulirish paulirish paulirish approved these changes

Assignees

@paulirish paulirish

Labels

breaking cla: yes waiting4reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

*Element artifacts should have devtoolsNodePath

5 participants

@adrianaixba @connorjclark @paulirish @googlebot @devtools-bot