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

Use fetch wrapper for DELETE requests to dispatch #578

Merged
merged 4 commits into from
Jan 27, 2020

Conversation

ashmaroli
Copy link
Member

Currently, a fetch wrapper for DELETE requests, is left unused.
Using the wrapper with slight modifications will bring consistency with GET and PUT requests that are sent via respective wrappers.

)
.then(data => {
dispatch({ type: action_success.type });
dispatch(action_success.update);
Copy link
Member

Choose a reason for hiding this comment

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

Why are there 2 dispatch here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mertkahyaoglu Because the original logic sends two dispatches. For example:

export const deleteDraft = (directory, filename) => dispatch => {
  return fetch(draftAPIUrl(directory, filename), {
    method: 'DELETE',
    credentials: 'same-origin',
  })
    .then(data => {
      dispatch({ type: DELETE_DRAFT_SUCCESS });
      dispatch(fetchDrafts(directory));
    })
    .catch(error =>
      dispatch({
        type: DELETE_DRAFT_FAILURE,
        error,
      })
    );

Copy link
Member

Choose a reason for hiding this comment

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

Oh, is it because we are not updating state, just refetching all drafts again, right? That's bad actually 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm.. this double dispatch could also be the reason behind the race condition issues with the frontend layer and the Ruby backend.. not sure though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this double-dispatch in b11903b

@ashmaroli ashmaroli removed the request for review from mertkahyaoglu January 27, 2020 07:25
@ashmaroli ashmaroli merged commit 043642d into jekyll:master Jan 27, 2020
@ashmaroli ashmaroli deleted the del-fetch-wrapper branch January 27, 2020 07:27
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