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

Client side delete #1

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

hjdivad
Copy link
Contributor

@hjdivad hjdivad commented Nov 2, 2017

No description provided.

@hjdivad
Copy link
Contributor Author

hjdivad commented Nov 2, 2017

@workmanw you'll notice the error manifests on this pr but not if you yarn link to emberjs/data#5249

@workmanw
Copy link
Owner

workmanw commented Nov 2, 2017

Hey @hjdivad Thanks for this! I'm going to take what you've got and add a commit on top to create a second button for this so we can demo both behaviors.

@workmanw workmanw merged commit 67b2f5f into workmanw:master Nov 3, 2017
@hjdivad
Copy link
Contributor Author

hjdivad commented Nov 3, 2017

@workmanw 👍

Although we're going to land a fix to get the semantics of unloadRecord for sync relationships back to client-side delete so you won't need to do this.

In either case, having an adapter-enabled client side delete is definitely public API and should work.

@hjdivad hjdivad deleted the hjdivad/client-side-delete branch November 3, 2017 17:04
@workmanw
Copy link
Owner

workmanw commented Nov 4, 2017

@hjdivad Is there a PR for that I can track? It's not emberjs/data#5249 right? No problem if it's not yet started. I'm just trying to make sure I'm following along.

@hjdivad
Copy link
Contributor Author

hjdivad commented Nov 8, 2017

@workmanw i'm going to merge that PR; the only thing I'll do before merging is trace the local delete case to confirm that the case is actually covered as @runspired believes.

I'll open a new pr for restoring the semantics completely (this work was partially done earlier). When I do I'll tag emberjs/data#5136 so you should get notified if you're following that issue.

@workmanw
Copy link
Owner

workmanw commented Nov 8, 2017

👍 Thanks a lot.

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.

2 participants