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_query and use_mutation #86

Merged
merged 60 commits into from
Sep 15, 2022
Merged

use_query and use_mutation #86

merged 60 commits into from
Sep 15, 2022

Conversation

rmorshea
Copy link
Contributor

@rmorshea rmorshea commented Jul 2, 2022

Description

Adds use_query and use_mutation hooks. I haven't tested to see that these work as expected, but they get across the interface that I think we should provide for writing database operations.

Checklist:

Please update this checklist as you complete each item:

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes, if necessary.
  • GitHub Issues which may be closed by this PR have been linked.

@rmorshea rmorshea force-pushed the use_database branch 6 times, most recently from 06fa974 to a863ad3 Compare July 2, 2022 19:33
@rmorshea rmorshea mentioned this pull request Jul 2, 2022
@Archmonger Archmonger linked an issue Jul 3, 2022 that may be closed by this pull request
@Archmonger Archmonger mentioned this pull request Jul 4, 2022
3 tasks
@Archmonger
Copy link
Contributor

  • Recursively for loop iterate through each object value to force run the query while within the hook
  • Have a flag to disable immediate_fetch
  • Double check the re-run workflow to ensure we aren't unnecessarily performing ORM queries

@rmorshea
Copy link
Contributor Author

rmorshea commented Jul 15, 2022

@Archmonger so the implementation I went with uses Model.get_deferred_fields and just calls getattr on all the field names it returns. I'm pretty sure this does what we want. I also require that if fetch_deferred_fields=True, then the query function must return a QuerySet or Model because it would be too hard to try and discover and fetch the deferred fields of Model instances nested inside arbitrary data structures.

@rmorshea
Copy link
Contributor Author

@Archmonger I've got to switch and work on some other things for a bit. If you're able to help get this over the finish by adding some docs or tests that would be great.

@Archmonger
Copy link
Contributor

Will do, but I am currently out on vacation. I'll get to it sometime after the 20th of this month.

src/django_idom/utils.py Outdated Show resolved Hide resolved
src/django_idom/hooks.py Show resolved Hide resolved
src/django_idom/hooks.py Outdated Show resolved Hide resolved
@Archmonger
Copy link
Contributor

Archmonger commented Jul 26, 2022

While I was writing out the docs I had a thought. Is there really any point to having separate use_query and use_mutation hooks?

I feel like the use_query hook could be renamed to use_orm and fill the role of both. For example, if the user doesn't return anything within their query function, we can just assume it's a mutation.

Perhaps that's just me not understanding the benefits of refetch=... though.

@Archmonger
Copy link
Contributor

Yep, just tested that as well. Looks like the server they use to host dev docs is down. I might switch the docs off of the dev branch if this becomes a reoccurring issue.

@rmorshea
Copy link
Contributor Author

rmorshea commented Sep 13, 2022

Ok, am able to repro the problem under 0.40.1 albeit with a slightly different error message:

Uncaught (in promise) TypeError: obj is undefined
    replace
    applyOperation
    applyPatch
    applyNonMutativePatch
    applyPatch
    onmessage

However, under proposed changes in bypass-jsonpatch that problem goes away:

Peek 2022-09-13 01-07

@Archmonger
Copy link
Contributor

I just tested out the current branch again, with bypass-jsonpatch. The following errors were given on three separate mouse clicks.

client.js:29997 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'id')
    at Object.replace (client.js:29997:26)
    at applyOperation (client.js:30206:60)
    at Object.applyPatch (client.js:30250:22)
    at applyNonMutativePatch (client.js:30649:20)
    at client.js:30633:20
    at client.js:31298:48
replace @ client.js:29997
applyOperation @ client.js:30206
applyPatch @ client.js:30250
applyNonMutativePatch @ client.js:30649
(anonymous) @ client.js:30633
(anonymous) @ client.js:31298
Promise.then (async)
socket.onmessage @ client.js:31298

client.js:30213 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'children')
    at applyOperation (client.js:30213:22)
    at Object.applyPatch (client.js:30250:22)
    at applyNonMutativePatch (client.js:30649:20)
    at client.js:30633:20
    at client.js:31298:48
applyOperation @ client.js:30213
applyPatch @ client.js:30250
applyNonMutativePatch @ client.js:30649
(anonymous) @ client.js:30633
(anonymous) @ client.js:31298
Promise.then (async)
socket.onmessage @ client.js:31298

client.js:3636 Warning: Encountered two children with the same key, `9`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted  the behavior is unsupported and could change in a future version.
    at ul
    at StandardElement (http://127.0.0.1:8000/static/django_idom/client.js:31094:28)
    at Element (http://127.0.0.1:8000/static/django_idom/client.js:31076:20)
    at StandardElement (http://127.0.0.1:8000/static/django_idom/client.js:31094:28)
    at Element (http://127.0.0.1:8000/static/django_idom/client.js:31076:20)
    at div
    at StandardElement (http://127.0.0.1:8000/static/django_idom/client.js:31094:28)
    at Element (http://127.0.0.1:8000/static/django_idom/client.js:31076:20)
    at StandardElement (http://127.0.0.1:8000/static/django_idom/client.js:31094:28)
    at Element (http://127.0.0.1:8000/static/django_idom/client.js:31076:20)
    at StandardElement (http://127.0.0.1:8000/static/django_idom/client.js:31094:28)
    at Element (http://127.0.0.1:8000/static/django_idom/client.js:31076:20)
    at StandardElement (http://127.0.0.1:8000/static/django_idom/client.js:31094:28)
    at Element (http://127.0.0.1:8000/static/django_idom/client.js:31076:20)
    at Layout (http://127.0.0.1:8000/static/django_idom/client.js:31060:19)

@Archmonger
Copy link
Contributor

I did some inspection of the code in my venv, seems like the git+ install wasn't overriding my local idom. I manually applied the patch to serve.py and it resolved the issue.

@rmorshea rmorshea marked this pull request as ready for review September 13, 2022 23:42
src/django_idom/websocket/consumer.py Outdated Show resolved Hide resolved
src/js/package.json Outdated Show resolved Hide resolved
tests/test_app/tests/test_components.py Outdated Show resolved Hide resolved
src/django_idom/hooks.py Outdated Show resolved Hide resolved
@Archmonger
Copy link
Contributor

Should I commit the from channels.auth import login and TransactionTestCase changes I suggested above?

@rmorshea
Copy link
Contributor Author

Go for it!

@Archmonger
Copy link
Contributor

LGTM, ready for merge.

@rmorshea rmorshea merged commit 1c250cb into main Sep 15, 2022
@Archmonger Archmonger deleted the use_database branch September 15, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants