Skip to content

Spread resolved data on updateWith()#669

Closed
marcoscaceres wants to merge 8 commits intogh-pagesfrom
spread_on_update
Closed

Spread resolved data on updateWith()#669
marcoscaceres wants to merge 8 commits intogh-pagesfrom
spread_on_update

Conversation

@marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Jan 19, 2018 •
edited by pr-preview bot
Loading

This builds on #668 to allow incremental data updates as proposed in #649.

Preview | Diff

@marcoscaceres marcoscaceres changed the title Spread on update Spread resolved data on updateWith() Jan 19, 2018
@marcoscaceres marcoscaceres requested a review from domenic January 19, 2018 05:20
@marcoscaceres
Copy link
Member Author

marcoscaceres commented Jan 19, 2018

@domenic, the interesting stuff only happens in fc5ea16. The rest is just #668.

@marcoscaceres marcoscaceres mentioned this pull request Jan 19, 2018
Merged
domenic
domenic requested changes Jan 19, 2018
View reviewed changes
Copy link
Collaborator

@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.

So it's better to stay at the Infra level than to drop down to the ECMAScript level. Otherwise what happens here will be different if someone monkeys with Object.prototype, for example. You'll need to define some kind of ordered map merging operation, or let us do that for you in Infra.

BUT, as I pointed out in #649 (comment), I don't think this patch is necessary at all. Let's continue discussing there.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Jan 22, 2018

Confirmed, not needed. Sorry about the noise. Someone needs to fire the person writing test tho :)

@marcoscaceres marcoscaceres deleted the spread_on_update branch January 22, 2018 05:07
@marcoscaceres
Copy link
Member Author

marcoscaceres commented Jan 24, 2018

@domenic, I'm still kinda wondering if we should add "property spread" to infra (even if we didn't need it here).

Consider, after validation the values object, it would be nice to just spread them over request.[[details]] instead of adding/replacing the properties one by one.

@domenic
Copy link
Collaborator

domenic commented Jan 24, 2018

Yeah, I definitely think some kind of "merge-with-overwrite" for ordered maps would be reasonable. I'd prefer to wait until we have a consumer, but we could open a tracking issue perhaps.

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

Reviewers

@domenic domenic domenic requested changes

Assignees

No one assigned

Labels

needs tests proposal - needs discussion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@marcoscaceres @domenic