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

feat(query): 🔥 add injectMutation to query #121

Merged
merged 7 commits into from
Nov 7, 2023

Conversation

luii
Copy link
Contributor

@luii luii commented Nov 5, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Copy link

stackblitz bot commented Nov 5, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

);

return {
mutate: mutationObserver.mutate.bind(mutationObserver),
Copy link
Member

Choose a reason for hiding this comment

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

What about mutateAsync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the mutate would be the mutateAsync here. if the normal mutate is also needed then i can implement it tomorrow though

Copy link
Member

Choose a reason for hiding this comment

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

the mutate would be the mutateAsync here

What do u mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just not named it mutateAsync, the mutate function just ignores the promise returned by the mutate function of the mutationObserver and attaches a noop on the catch block of the promise. If you say that we'd need this additional function then im gonna implement it tomorrow, i just thought that it doesnt yield any advantage, so i didnt have implemented it for now, since you can just ignore the promise returned and it would still work.

query/src/lib/mutation.ts Outdated Show resolved Hide resolved
query/src/lib/mutation.ts Show resolved Hide resolved
src/app/mutation-page/mutation-page.component.ts Outdated Show resolved Hide resolved
src/app/mutation-page/mutation-page.component.ts Outdated Show resolved Hide resolved
src/app/mutation-page/mutation-page.component.ts Outdated Show resolved Hide resolved
@NetanelBasal
Copy link
Member

Can you resolve the conflicts please?

@NetanelBasal
Copy link
Member

I'll merge this and maybe make some changes.I think I'll change the toSignal function to result() instead

@NetanelBasal NetanelBasal merged commit 9f136d2 into ngneat:next Nov 7, 2023
1 check passed
@luii
Copy link
Contributor Author

luii commented Nov 7, 2023

I'll merge this and maybe make some changes.I think I'll change the toSignal function to result() instead

yeah i think this would sound good, though it maybe could be a bit obscure for what it means if one does not read the documentation beforehand.

Anyway, do you think there are some more thinks i can do? If there is nothing else than the documentation and playground examples i'll continue with the docs tomorrow then.

@NetanelBasal
Copy link
Member

Docs and It'll be awesome if you can make the playground looks better maybe use bulma so that we can include a gif in the release tweet :)

@luii
Copy link
Contributor Author

luii commented Nov 7, 2023

Of course, i'll start with that tomorrow then!

@luii luii deleted the feat/mutation-api branch November 7, 2023 19:11
@NetanelBasal
Copy link
Member

Checkout this commit:

093e053

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.

3 participants