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

Advanced Websocket Features #17

Merged
merged 39 commits into from
Nov 1, 2021

Conversation

Archmonger
Copy link
Contributor

@Archmonger Archmonger commented Sep 21, 2021

New Features

  • Automatic WS reconnection (settings.py:IDOM_WS_RECONNECT_TIMEOUT)
  • Propogate WS down to components
  • Support authenticated components (Login the user and save the session if auth middleware is detected)
  • Pin "idom-client-react": "^0.33.3"
  • Update tests to add websocket as a component parameter
  • Update tests to use render shortcut instead of HttpResponse
  • Change ALLOWED_HOSTS = ["*"] in tests to allow LAN access
  • Rename IdomAsyncWebSocketConsumer -> IdomAsyncWebsocketConsumer
  • Readme clean up. fixes, and updates

@Archmonger
Copy link
Contributor Author

if I don't complete this PR by Thursday, I likely won't be available to pick it back up until October 4th.

@Archmonger
Copy link
Contributor Author

Archmonger commented Sep 22, 2021

Reconnecting WS has been developed.

Currently works on client side disconnect.

There are some caveats...

  1. Reconnect is not graceful.
    • All IDOM modules are effectively "reset" to the first rendered state upon reconnect.
    • All variable values are lost.
    • We may need to implement a database model to store the component state in relation to a Websocket session.
      • Perhaps this should be a new decorator, such as @persistent_component
  2. Reconnections after server restart is broken due to Django IDOM design.
    • We only add modules to IDOM_REGISTERED_COMPONENTS upon template first render, so all components are completely unknown after server restart ("Unknown IDOM view ID").
    • We will need to develop a feature to resolve this. I'll get to it eventually.
  3. As you mentioned before the alert in IDOM core is annoying and needs to be removed.

@Archmonger Archmonger self-assigned this Sep 22, 2021
@Archmonger Archmonger linked an issue Sep 22, 2021 that may be closed by this pull request
@Archmonger Archmonger marked this pull request as ready for review September 22, 2021 06:46
@Archmonger Archmonger requested a review from a team as a code owner September 22, 2021 06:46
@Archmonger
Copy link
Contributor Author

Archmonger commented Sep 22, 2021

If approved, the last limb of Conreq blocking factors is react-router support.

Unfortunately I'm too much of a ReactJS scrub to PR that into IDOM Core, so I'll need your support on that.


Side note

Before we actually start getting repos with django-idom dependencies, I believe we should change IdomAsyncWebSocketConsumer to have a lowercase socket to fall in line with Django conventions (ex AsyncJsonWebsocketConsumer).

What are your thoughts?

@Archmonger
Copy link
Contributor Author

Archmonger commented Oct 6, 2021

Let me know what additional changes are needed in this PR.

@rmorshea
Copy link
Contributor

rmorshea commented Oct 9, 2021

Been really busy lately. Will try to get to this next week.

@Archmonger
Copy link
Contributor Author

I updated the PR description.

Let me know if you need me to split this PR into separate parts to be more easily digestible.

Copy link
Contributor

@rmorshea rmorshea left a comment

Choose a reason for hiding this comment

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

LGTM - just minor nits.

Once I implement the use_context hook I think we'll be able to get rid of this websocket parameter and just allow the user to access the socket from anywhere:

@component
def MyComponent():
    socket = use_context(WebSocketContext)
    ...

With everything else that needs to happen that's probably a ways off though so this should be fine for the time being.

src/django_idom/templates/idom/component.html Outdated Show resolved Hide resolved
src/django_idom/websocket_consumer.py Outdated Show resolved Hide resolved
src/django_idom/websocket_consumer.py Outdated Show resolved Hide resolved
src/django_idom/websocket_consumer.py Outdated Show resolved Hide resolved
tests/test_app/templates/base.html Outdated Show resolved Hide resolved
src/django_idom/config.py Outdated Show resolved Hide resolved
@Archmonger
Copy link
Contributor Author

Archmonger commented Nov 1, 2021

Side note...

We should also consider adding pre-commit.ci to all idom-team repos. I see that IDOM Core is using it currently, but it's using a fairly minimal config.

You can just take my extensive pre-commit.ci configuration file and add it in.

@Archmonger
Copy link
Contributor Author

All comments have been resolved.

@rmorshea
Copy link
Contributor

rmorshea commented Nov 1, 2021

I see that IDOM Core is using [pre-commit] currently, but it's using a fairly minimal config.

From my experience, pre-commit has been most helpful in resolving issues that can be fixed automatically (e.g. black, sorting imports) which is why I've kept it fairly minimal. Linters like flake8 and pylint that just report errors can be annoying if you want to introduce a commit that represents intermediate progress. So including pre-commit hooks that automatically add EOF new lines and strip out trailing white space would be great to have.

A longer-term solution to this would be to make an idom-team/python-package-template repo that pre-bakes all the standards we want to enforce.

@Archmonger Archmonger merged commit 9336098 into reactive-python:main Nov 1, 2021
@Archmonger Archmonger deleted the more-ws-features branch November 1, 2021 19:22
@Archmonger
Copy link
Contributor Author

A longer-term solution to this would be to make an idom-team/python-package-template repo that pre-bakes all the standards we want to enforce.

I recommend using the repo-file-sync GitHub action to automate standardized configurations across idom-team repos.

That's what we use at Jazzband alongside our template repository.

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

Successfully merging this pull request may close these issues.

2 participants