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

core(artifacts): encapsulate node details in an object (#11474) (reland) #11695

Merged
merged 9 commits into from
Dec 8, 2020

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Nov 20, 2020

This is a reland of #11474

See #11694 for revert details.

It's already reviewed and LGTM'd.

Update: merging master in introduced quite a few conflicts (brendan's nodevalue type change, @adrianaixba's ImageElements resourcesize change, etc), so in resolving the conflicts, I did make enough changes to warrant a quick once-over.

Notably, I changed artifacts.json quite a bit from where it was in #11474. I optimized for the smallest amount of changes to sample_v2.json, and am happy with the results (just ~4 lines changed to that file).

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

I think we have one too many revert in here or something @paulirish ? 0/0 changes :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

I didn't find any merge issues in my look through 👍 LGTM

EDIT: i.e. the changes I noticed seemed fine, definitely appreciate the artifacts tuning to reduce sample.json changes :)

@patrickhulce patrickhulce merged commit 2cbbd28 into master Dec 8, 2020
@patrickhulce patrickhulce deleted the relandencap branch December 8, 2020 02:58
@adrianaixba
Copy link
Collaborator

Nice! Thanks for cleaning this up, it was a hefty PR! 🎉

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

Successfully merging this pull request may close these issues.

4 participants