8000 chore: update tests for replaceItems with actOnDispatch by jd1378 · Pull Request #752 · feathersjs-ecosystem/feathers-hooks-common · GitHub
[go: up one dir, main page]
Skip to content

chore: update tests for replaceItems with actOnDispatch#752

Merged
fratzinger merged 1 commit intofeathersjs-ecosystem:masterfrom
jd1378:add_tests_for_replace_items_dispatch
Jun 10, 2024
Merged

chore: update tests for replaceItems with actOnDispatch#752
fratzinger merged 1 commit intofeathersjs-ecosystem:masterfrom
jd1378:add_tests_for_replace_items_dispatch

Conversation

@jd1378
Copy link
Contributor
@jd1378 jd1378 commented Jun 10, 2024 •
edited
Loading

Summary

Tests for #751

@jd1378 jd1378 mentioned this pull request Jun 10, 2024
Merged
@fratzinger
Copy link
Collaborator
fratzinger commented Jun 10, 2024

Wow. Thanks for the fast delivery! 💚 Looks good. The only see I wonder about is 'NOTE THIS TEST NOTE THIS TEST' in one of the tests? Is this a leftover or intentional?

@jd1378
Copy link
Contributor Author
jd1378 commented Jun 10, 2024 •
edited
Loading

well I just copied the test block above it and modified it for dispatch, but It looks like an important test so it might be intentional (?). but I did not add that part for sure

@fratzinger fratzinger merged commit fc9f5e4 into feathersjs-ecosystem:master Jun 10, 2024
@fratzinger
Copy link
Collaborator
fratzinger commented Jun 10, 2024

lol. merged it then. I want to work on a few things in this repo anyways. I don't like the implicit magic that happens around replaceItems. When to use context.data, when to use context.result depending on context.type? It does not work for around hooks at all? What do you want to achieve with replaceItems in a before hook if there already is context.result because you want to skip the adapter call.

Too many questions and to much magic for my taste. I want to introduce a more explicit replaceData and replaceResult and maybe replaceDispatch or get that covered by replaceResult(..., { useDispatch: true | false | 'ifExistent' }) or something. Just thinking out loud.

That said I'll eventually rework this stuff anyways.

@jd1378
Copy link
Contributor Author
jd1378 commented Jun 10, 2024 •
edited
Loading

hmm, I found the bug for replaceItems when I was using feathers-casl. it was using replaceItems but things were not getting replaced properly* (feathers-pinia had a bug where it couldn't handle arrays, it's fixed now). so It took me some time to pin point it to the dispatch issue. and then I had to patch feathers-casl to replace both result and dispatch.
I have done A LOT of modifications to feathers-casl to get it working the way I want it to, and it's still not what I expect.
I know it's not the place to discuss it, just wanted to mention where this little utility was having impact on my project.
(btw feathers-casl's most interesting feature for me was the getChannelsWithReadAbility function, which worked very good)

@jd1378 jd1378 deleted the add_tests_for_replace_items_dispatch branch June 10, 2024 12:34
@fratzinger
Copy link
Collaborator
fratzinger commented Jun 10, 2024

Wrote you on discord. I would love to chat about this stuff. Not now but in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jd1378 @fratzinger
0