Skip to content

Conversation

Archmonger
Copy link
Contributor

@Archmonger Archmonger commented Oct 29, 2021

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
Copy link
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
Copy link
Contributor Author

Converting to draft while I debug django-silk compatibility

@Archmonger
Copy link
Contributor Author

Archmonger commented Oct 30, 2021

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
@Archmonger
Copy link
Contributor Author

Archmonger commented Nov 1, 2021

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.

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 besides a few more minor nits

@rmorshea
Copy link
Contributor

rmorshea commented Nov 1, 2021

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

@Archmonger
Copy link
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
Labels
None yet
2 participants