8000 fix(utils): deepMixIn to properly merge undefined values of source by ivanvoznyakovsky · Pull Request #502 · js-data/js-data · GitHub
[go: up one dir, main page]
Skip to content

fix(utils): deepMixIn to properly merge undefined values of source #502

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
merged 3 commits into from
Jun 22, 2018
Merged

fix(utils): deepMixIn to properly merge undefined values of source #502

merged 3 commits into from
Jun 22, 2018

Conversation

ivanvoznyakovsky
Copy link
Contributor

Fixes #501

  • - npm test succeeds
  • - Code coverage does not decrease (if any source code was changed)
  • - If applicable, appropriate JSDoc comments were updated in source code (if applicable)
  • - If applicable, approprate changes to js-data.io docs have been suggested ("Suggest Edits" button)

@codecov
Copy link
codecov bot commented May 1, 2018 •
edited
Loading

Codecov Report

Merging #502 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
+ Coverage   95.83%   95.84%   +<.01%     
==========================================
  Files          21       21              
  Lines        2524     2525       +1     
==========================================
+ Hits         2419     2420       +1     
  Misses        105      105
Impacted Files Coverage Δ
src/utils.js 96.85% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78fcdc0...53d3de4. Read the comment docs.

@crobinson42 crobinson42 merged commit 2fb5bf0 into js-data:master Jun 22, 2018
@zuzusik
Copy link
Contributor
zuzusik commented Jun 23, 2018 •
edited
Loading

this new behavior should be somehow optional

in some cases it can be required to remove properties that don't exist anymore - e.g. some prop was removed from the object and we receive updated object from the network

also a lot of code could have already rely on this behavior, so changing it may break it

@ivanvoznyakovsky
Copy link
Contributor Author

isn't onConflict config option is what you're talking about? setting it to replace would actually do what you describe and this change fixes the default merge strategy.
but anyway I do hear what you say and this change should definitely be marked as breaking change since it can break some code that actually relied on buggy merge strategy.

@crobinson42
Copy link
Member

@zuzusik I think you might be right - this could cause potential breaking behavior in existing implementations. I'll look at possibly making this an option with some flag. If you have any ideas, throw them out.

@crobinson42
Copy link
Member

@ivanvoznyakovsky onConflict only applies to one use of deepMixin() in Collection.js, inside Record.js the save() method also uses deepMixin() without onConflict.

@ivanvoznyakovsky
Copy link
Contributor Author

@crobinson42 I should have missed it. then we should definitely think of making it optional.

@crobinson42
Copy link
Member

After looking at at example a little further and taking into consideration @zuzusik comment about this breaking existing implementations - the code snippet in the issue doesn't correctly depict the behavior, @ivanvoznyakovsky - the key still exists but is overwritten with the value undefined. I'm going to lean towards maintaining the deepMixin() existing behavior.

crobinson42 added a commit that referenced this pull request Jun 23, 2018
@crobinson42
Revert "fix(utils): deepMixIn to properly merge undefined values of s…
5422ec5 
…ource (#502)"

This reverts commit 2fb5bf0.
@crobinson42 crobinson42 mentioned this pull request Jun 23, 2018
Merged
@crobinson42
Copy link
Member

@ivanvoznyakovsky here is an example of the existing behavior without this PR: https://runkit.com/crobinson42/github-com-js-data-js-data-issues-501

@ivanvoznyakovsky
Copy link
Contributor Author

actual problem is how record object is created from api response based on record schema.
say we have the following schema for a record

{
  schema: {
    properties: {
      uid: {type: 'string'},
      name: {type: 'string'}, 
      comment: {type: 'string', 'null'}
    }
  }
}

comment is only returned when queried explicitly with a url param, say withComment.
now there are 2 components that make 2 requests in parallel. one does a request with withComment param the other one does not need a comment hence skips withComment param.
if a request that was done without withComment param resolves after the one with withComment we'll get in the following situation:

  • the record is created from response data {uid: 1, name: 'some'} based on schema {uid: 1, name: 'some', comment: undefined}
  • this record is merged with the one in store, eg. {uid: 1, name: 'some', comment: 'a comment'}, resulting in store record {uid: 1, name: 'some', comment: undefined}
  • component that requires comment field now fails to show it although it requested it and it was in the response.
    this is the exact problem we're facing in our app.

@ivanvoznyakovsky
Copy link
Contributor Author

as @zuzusik said we could potentially make skipping undefined fields of source optional. say having a config option skipUndefinedOnMerge that is false by default. which would be the same as it is now.

@crobinson42
Copy link
Member

@ivanvoznyakovsky I understand your situation - thanks for outlining that. I don't disagree with making a flag to skip undefined fields. If you've got some ideas on how to integrate that cleanly, throw them out :-)

@ivanvoznyakovsky
Copy link
Contributor Author
ivanvoznyakovsky commented Jun 23, 2018 •
edited
Loading

thoughts on creating a new merge strategy mergeSkipUndefined? we'll still keep merge as default one but could just override it when needed via onConflict value.
deepMixIn would get a 3rd param skipUndefined.

@crobinson42
Copy link
Member

I think the problem is in the way the schema is applied - perhaps we should look at a different angle of solving this.

I don't think the situation you described above where a query is made without explicitly defining a key, then that key shouldn't be in the source object to be merged, ie: if withComment isn't defined, there should not be a key at all in the object.

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

@crobinson42 crobinson42 crobinson42 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivanvoznyakovsky @zuzusik @crobinson42
0