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

[FEATURE] Return pushPayload result by default #5265

Closed
wants to merge 1 commit into from

Conversation

Exelord
Copy link

@Exelord Exelord commented Nov 27, 2017

Hey, after almost 2 years, are we ready to move further with #4110 ? :)

This PR removes ds-pushpayload-return feature flag and enables this behavior by default.

I would love to merge it, so if any further work is required just give me a sign.

@workmanw
Copy link

As the original author of ds-pushpayload-return I'm 👍 on this. But I think about 8 months or so ago someone told me this feature was a no go because it restricted future performance improvements that could eventually be made to pushPayload.

That said, hopefully a core team member can chime in and tell us definitively.

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 9, 2018

@bmac Are there any discussions on this one ? I'm personnally 👍 but, not aware of the restricted future performance improvementsthing...

@bmac
Copy link
Member

bmac commented Mar 9, 2018

Its been a while but I believe the issue was there was a plan to defer the work of creating records until they were needed by the user. Returning them here would force them to always be created by push payload.

I think @igorT had some ideas about making introducing a new api to make this more granular so the user could be specific about if they wanted to "Just push these records into the store and update the source of truth" or "push these records into the store and give me something back".

@Exelord
Copy link
Author

Exelord commented Mar 26, 2018

OK... So we have to wait for the new API :/

@Exelord Exelord deleted the feature-pushpayload-return branch March 26, 2018 12:52
@sly7-7
Copy link
Contributor

sly7-7 commented Mar 26, 2018

@Exelord For now a quite straightforward workaround could be: #5384 (comment)

@Exelord
Copy link
Author

Exelord commented Mar 26, 2018

It's cool but it still assume that I know what is in the response :/ which I dont, tahts why pushPayload was the only way here

@runspired
Copy link
Contributor

@Exelord actually it doesn't assume. In fact quite the opposite. store.pushPayload assumed an incredible amount about the payloads it was receiving, usually incorrectly. Users had to massage the payload into a format that was totally different format from what their API gave them and from what their serializer expected just to use the feature. Meanwhile the util method based over normalizeResponse give you the exact information you need when you need it for how to properly convert the data to json-api format.

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.

5 participants