Skip to content

[dont merge] - Refactor proxy client & migrate issue screen #770

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

Closed
wants to merge 8 commits into from

Conversation

machour
Copy link
Member

@machour machour commented Apr 12, 2018

Description

Hi team,

I've started looking at Step 4 of #761 (handle put/patch/delete) and experimented on the Issue screen.

While doing that, I came to the conclusion that the Proxy <=> Client interactions needed to be changed a bit, in order to give the proxy more power over all kinds of operations, and how the resulting data is reduced in store.

As a result, the client/proxy code is way clearer than before 🎉

You can follow the train of thought that lead to this PR in this gist, and I really encourage you to do so in order to completely understand this Proxy thing: https://gist.github.com/machour/698e91e075ac35ad64531d03c80a99bf

If you pull this PR and test it locally, you will see the following stuff:

  • We retrieve the comments for an issue
  • If we add a comment, a POST call is triggered, and its result is automatically appended to the list of comments (without refetching)
  • If we edit a comment, a PATCH call is triggered, and the changed fields are automatically updated in store (without refetching)
  • If we delete a comment, a DELETE call is triggered, and the comment reference is removed from the comments list in store (again, no refetching)
  • If you lock/unlock the issue, a PUT call is triggered, and the Issue object we already have in store is patched so that the locked property is up to date (instead of re-fetching the whole issue).

Same thing goes for issue labels and assignees.

To say thing differently: Our Store is 100% up to date with GitHub, by doing precisely what needs to be done. Nothing more. Nothing less.


This PR won't be merged as a whole.
Instead, I'll be opening a smaller PR with the subset of changes needed for the refactoring so that we can confirm that everything still work without touching the migrated screens code.

Once this first PR is merged, I'll open a new one to unleash the PUT/DELETE/PATCH/POST features.


I've already did a 1 on 1 with @housseindjirdeh to walk him through this PR, and I'll be happy to do this with any of you girls and guys!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 42.287% when pulling ac5a30e on machour:refactor-proxy-client into 4b32a92 on gitpoint:master.

@machour
Copy link
Member Author

machour commented Apr 22, 2018

Closing this demo.

@machour machour closed this Apr 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants