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 useUpdate hook and improve defaultMutations and mutators dependency #62

Merged
merged 10 commits into from
Jul 13, 2021

Conversation

Timi-Duban
Copy link
Collaborator

@Timi-Duban Timi-Duban commented Jul 9, 2021

Two big things here:

  • Autorization stuff and document's existence checking moved from mutation to mutator to be clearer on database access.
  • Fix useUpdate hook that didn't pass the Id when provided

This is a draft PR because I need to think about all the TODO on these documents, and to fix and improve the tests that are currently broken because of the changes.

About #33 and maybe #59 it needs to be tested
Closes #33

@Timi-Duban
Copy link
Collaborator Author

Just did a todo: data validation functions where splitted in 2 (validateDocument and validateData) that were called by validateMutationData in a tricky way in the mutator.
Now I think it's clearer and safer, tests are updated too and they're all good.

@eric-burel
Copy link
Collaborator

Just did a todo: data validation functions where splitted in 2 (validateDocument and validateData) that were called by validateMutationData in a tricky way in the mutator.
Now I think it's clearer and safer, tests are updated too and they're all good.

Great work so far!!
Yes indeed this function was dirty. Just add a comment at the top of the file explaning this change: some old applications may use those functions in their own code. I am not sure, so at least add a comment saying smth like "Differences with vulcan Meteor: - removed validateDocument and refactored validateData, so now we use only validateData" so we can remember this modification compared to Vulcan Meteor.

@Timi-Duban Timi-Duban marked this pull request as ready for review July 12, 2021 14:58
async function getSelector({ dataId, selector, input, context, model }: GetSelectorInput
) {
if (dataId) {
selector = { _id: dataId };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer early return pattern:
if (dataId) return ...
if (selector) { deprecate(); return ... }
return ...

@eric-burel
Copy link
Collaborator

@Timi-Duban I think we're good, the last comments are pure form but it seems that you have covered everything at this point. Does it work ok when you plug Vulcan Next?
If yes I'll merge that

Thanks for this huge refactor!

@eric-burel
Copy link
Collaborator

🥇

@eric-burel eric-burel merged commit 1ba1377 into VulcanJS:main Jul 13, 2021
@Timi-Duban Timi-Duban deleted the fix-mutation-hooks branch July 13, 2021 15:02
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.

Redundancy between mutators and defaultMutations?
2 participants