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

Add Django CSS/JS de-duplication #226

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Archmonger
Copy link
Contributor

@Archmonger Archmonger commented Feb 6, 2024

Description

Since CSS/JS is loaded as raw strings within a style tag, it's currently difficult to prevent duplicate loads.

This PR implements a simple server-side de-duplication algorithm for CSS and JS.

Checklist

Please update this checklist as you complete each item:

  • Tests have been developed for bug fixes or new functionality.
  • The changelog has been updated, if necessary.
  • Documentation has been updated, if necessary.
  • GitHub Issues closed by this PR have been linked.

By submitting this pull request I agree that all contributions comply with this project's open source license(s).

@Archmonger Archmonger changed the title Django CSS/JS load "Only Once" Add Django CSS/JS de-duplication Feb 8, 2024
Comment on lines 150 to 172
async def add_end_css(event):
set_css_files(
css_files
+ [
reactpy_django.components.django_css(
"django-css-only-once-test.css",
allow_duplicates=True,
key=str(uuid4()),
)
]
)

async def add_front_css(event):
set_css_files(
[
reactpy_django.components.django_css(
"django-css-only-once-test.css",
allow_duplicates=True,
key=str(uuid4()),
)
]
+ css_files
)
Copy link
Contributor Author

@Archmonger Archmonger Feb 11, 2024

Choose a reason for hiding this comment

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

@rmorshea Is changing a list supposed to cause all components in the list to re-render (unmount, remount) every time?

I know the identity of the list is changing, but it's still a list. Not sure if you intended for swapping out lists to cause everything to unmount and re-render.

Copy link
Contributor Author

@Archmonger Archmonger Feb 19, 2024

Choose a reason for hiding this comment

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

Did some more debugging and I've managed to characterize this issue. I forgot that use_effect(..., dependencies=None) cleans up after every render. I created an issue to see if there's viability in an alternative effect condition.

In the meantime, I tried creating a workaround but was unsuccessful in getting use_effect(..., dependencies=[]) unmount to trigger when the component gets removed from a list. This might actually be a significant bug and potential memory leak.

@component
def _django_static_file(
    static_path: str,
    prevent_duplicates: bool,
    file_type: str,
    vdom_constructor: VdomDictConstructor,
):
    scope = use_scope()
    mount_trigger, set_mount_trigger = hooks.use_state(True)
    ownership_uuid = hooks.use_memo(lambda: uuid4())

    # Configure the ASGI scope to track the file
    if prevent_duplicates:
        scope.setdefault("reactpy", {}).setdefault(file_type, {})
        scope["reactpy"][file_type].setdefault(static_path, ownership_uuid)

    # Check if other _django_static_file components have unmounted
    @hooks.use_effect(dependencies=None)
    async def mount_manager():
        if prevent_duplicates and not scope["reactpy"][file_type].get(static_path):
            scope["reactpy"][file_type].setdefault(static_path, ownership_uuid)
            set_mount_trigger(not mount_trigger)

    # Notify other components that we've unmounted
    @hooks.use_effect(dependencies=[])
    async def unmount_manager():
        if not prevent_duplicates:
            return

        def unmount():
            if scope["reactpy"][file_type].get(static_path) == ownership_uuid:
                scope["reactpy"][file_type].pop(static_path)

        return unmount

    # Render the component, if needed
    if not prevent_duplicates or (
        scope["reactpy"][file_type].get(static_path) == ownership_uuid
    ):
        return vdom_constructor(cached_static_contents(static_path))

@Archmonger Archmonger linked an issue Feb 13, 2024 that may be closed by this pull request
@Archmonger
Copy link
Contributor Author

@Archmonger
Copy link
Contributor Author

Note to self, once we bugfix the script element's duplicate renders, this can be implemented as a client-side feature.

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

Successfully merging this pull request may close these issues.

Allow django_css and django_js to load only_once
1 participant