Skip to content

Auto populate IDOM component registry - #24

Merged
Archmonger merged 23 commits into
reactive-python:mainfrom
Archmonger:populate-idom-components
Nov 2, 2021
Merged

Auto populate IDOM component registry#24
Archmonger merged 23 commits into
reactive-python:mainfrom
Archmonger:populate-idom-components

Conversation

@Archmonger

@Archmonger Archmonger commented Oct 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

When Django first loads, attempt to populate the IDOM component registry.

This design was inspired by django_compressor. See https://github.com/idom-team/django-idom/discussions/20 for more details.

@Archmonger
Archmonger requested a review from a team as a code owner October 29, 2021 05:38
@Archmonger Archmonger self-assigned this Oct 29, 2021
@Archmonger

Archmonger commented Oct 29, 2021

Copy link
Copy Markdown
Contributor Author

The only caveat to this design is it seems to inconsistently break django-silk.
Currently digging into root cause on that.

@Archmonger
Archmonger marked this pull request as draft October 30, 2021 03:59
@Archmonger

Archmonger commented Oct 30, 2021

Copy link
Copy Markdown
Contributor Author

Converting to draft while I debug django-silk compatibility

@Archmonger

Archmonger commented Oct 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It was too much of a hassle to PR a fix for django-silk falling apart due to non-HTTP request renders (silk's source is messy), so I opted to rely on regex to parse component names.

This implementation should be functionally equivalent.

@Archmonger
Archmonger marked this pull request as ready for review October 30, 2021 05:33
@Archmonger
Archmonger requested a review from rmorshea October 30, 2021 08:57
@Archmonger Archmonger linked an issue Oct 30, 2021 that may be closed by this pull request
Avoid multiple component registrations #5
Closed
rmorshea
rmorshea requested changes Nov 1, 2021
View reviewed changes
Comment thread src/django_idom/utils.py Outdated
Comment thread src/django_idom/utils.py Outdated
@Archmonger

Archmonger commented Nov 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I resolved the two comments, and additionally I've created a tests folder since we'll likely add more in the future.

Django mandates this test folder naming scheme, but it does feel a bit awkward though since our test project is also called tests. Might be something worth changing in a future PR.

rmorshea
rmorshea requested changes Nov 1, 2021
View reviewed changes

@rmorshea rmorshea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM besides a few more minor nits

Comment thread src/django_idom/utils.py Outdated
Comment thread tests/test_app/tests/test_regex.py Outdated
@rmorshea

rmorshea commented Nov 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Once we get this in I think we can do a release if the threading issue is gonna take a while to resolve.

@Archmonger

Archmonger commented Nov 1, 2021

Copy link
Copy Markdown
Contributor Author

Completed!

@Archmonger
Archmonger merged commit 0cd049e into reactive-python:main Nov 2, 2021
@Archmonger
Archmonger deleted the populate-idom-components branch November 2, 2021 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rmorshea rmorshea rmorshea approved these changes

Assignees

@Archmonger Archmonger

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Avoid multiple component registrations

2 participants

@Archmonger @rmorshea