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

Fix duplicate attributes issue #18

Merged
merged 4 commits into from
Nov 11, 2016

Conversation

amwmedia
Copy link
Contributor

@amwmedia amwmedia commented Dec 3, 2015

fixed an issue where a record that is see twice, ends up with an _attributes array that has all values duplicated

@beauby
Copy link
Owner

beauby commented Dec 6, 2015

Hi @amwmedia, thanks a lot for the fix. Would you mind adding a test case?

@amwmedia
Copy link
Contributor Author

amwmedia commented Dec 7, 2015

no problem. I've really enjoyed working with your library. If brings a lot of usefulness without trying to do too much. 👍

@@ -53,6 +53,11 @@ describe('JsonApiDataStore', () => {
expect(article.title).to.eq('Cool article');
expect(article.author).to.eq('Lucas');
});

it('should not duplicate the attributes if the record is processed again', () => {
var article = store.sync(payload);
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind explicitly syncing the payload twice here? There is no guarantee the tests will always be ran in the same order.

@beauby
Copy link
Owner

beauby commented Dec 31, 2015

Sorry I took so long to review this. Please see my last comment. Apart from that, it looks ready to be merged!

@amozoss
Copy link

amozoss commented Apr 23, 2016

Would be neat if this was merged. Seems like it just needs a one line change or something. @amwmedia

@beauby
Copy link
Owner

beauby commented Apr 23, 2016

Sorry, I've been a bit busy with other stuff lately but plan to take care
of this one tomorrow.

On Saturday, 23 April 2016, Dan Willoughby notifications@github.com wrote:

Would be neat if this was merged. Seems like it just needs a one line
change or something. @amwmedia https://github.com/amwmedia


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#18 (comment)

Lucas Hosseini
lucas.hosseini@gmail.com

@amwmedia
Copy link
Contributor Author

@amozoss yeah, there's really not a lot to do on this, I just haven't had ANY time. We are approaching an MVP release at work... and all that entails. once we get past this release, I'll have some time to contribute more. I'll button this up then if it's not been taken care of by someone else at that point.

@anlek
Copy link

anlek commented May 12, 2016

Any news on this?

@beauby
Copy link
Owner

beauby commented Oct 21, 2016

Hey @amwmedia – any news on this one?

@amwmedia
Copy link
Contributor Author

this should be ready now, sorry for all the delays. We had a major MVP going out on a tight deadline so everything else took a backseat.

@anlek
Copy link

anlek commented Nov 10, 2016

@amwmedia Hope your MVP went well. I was just wondering if there is any chance of this patch being released via NPM?

@amwmedia
Copy link
Contributor Author

This is ready to be merged. @beauby can you accept the PR? :-)

@beauby beauby merged commit b25b6de into beauby:master Nov 11, 2016
@beauby
Copy link
Owner

beauby commented Nov 11, 2016

Merging – thanks for your work @amwmedia!

@amwmedia amwmedia deleted the fix-duplicate-attributes-issue branch November 11, 2016 13:21
@anlek
Copy link

anlek commented Nov 11, 2016

That's awesome, thank you for merging. Any chance we can see this be released via NPM sometime soon?

niksy referenced this pull request in niksy/jsonapi-data-manager May 10, 2017
* fixed an issue where a record that is see twice, ends up with an _attributes array that has all values duplicated
* added test case for attribute duplication issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants