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

Ensure that marshalArgs pulls wildcard info value from __data #5476

Merged
merged 8 commits into from
Feb 4, 2019

Conversation

kevinpschaaf
Copy link
Member

It currently pulls the value from changedProps rather than __data, meaning it could provide stale data for re-entrant changes.

Reference Issue

Fixes #5475

It currently pulls the value from `changedProps` rather than __data, meaning it could provide stale data for re-entrant changes.
if (structured) {
let value = get(data, path);
// when data is not stored e.g. `splices`, get the changed value
if (value === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So we want to pull the live value here, but we fall back to the stored value in props for splices. Adding a specific check for splices would be best but might be a performance hit. What if we take the value in props only if it is not === undefined? This allows the live value to be undefined which it sometimes can be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added skipped tests and TODO to address this in a separate change.

Copy link
Contributor

@sorvell sorvell left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinpschaaf kevinpschaaf merged commit 5501554 into master Feb 4, 2019
@kevinpschaaf kevinpschaaf deleted the 5475-kschaaf-stale-wildcard branch February 4, 2019 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants