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

Proposal: add customizable app_client_class config value #138

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Niicck
Copy link
Collaborator

@Niicck Niicck commented Jun 21, 2024

Here's a solution that could give people the freedom to pull manifest.json files from any external location.

This was heavily inspired by a similar feature in django-webpack-loader: django-webpack/django-webpack-loader#210. It's addressing the same user issue that we have, some people wanted to load their manifests from external urls. Instead of supporting every bespoke source that a manifest.json could be generated from, django-webpack-loader added the ability for users to plug in their own loader classes that had their own retrieval mechanisms.

This PR gives an example of that in test_custom_app_client.py. (Admittedly, this implementation is a little messier than django-webpack-loader, since we have to go through a second static class.)

There are probably ways to make this cleaner. I'm open to any suggestions.

Related: #105

Copy link
Owner

@MrBin99 MrBin99 left a comment

Choose a reason for hiding this comment

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

I think it's a very good idea to let people change how to load and parse the manifest file as it changes a lot between Vite versions and project configurations.
Maybe can you add a short paragraph in the README to let people know that and I will be happy to approve this change.

Thanks !

@@ -212,6 +222,8 @@ class DjangoViteAppClient:
DjangoViteConfig provides the arguments for the client.
"""

ManifestClient = ManifestClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems a bit redundant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Defining the ManifestClient as a class variable allows you to create a subclassed DjangoViteAppClient with a separate subclassed ManifestClient. You can look at the test_custom_app_client_class for an example. If we didn't do this, then we'd have no way to overwrite the logic of the ManifestClient without doing module-level monkeypatching.

Thinking this through, we should probably also add other dependency classes like TagGenerator as class variables as well, to allow them to be easily extensible.

@bbrowning918
Copy link

This worked really well to prove out that we could move from webpack/django-webpack-loader to vite/django-vite. It was smooth sailing with the app_client_class setting override.

As an idea for simplification, if manifest_path was able to discern/check for a URL or Path (or some other combination with a manifest_url setting) would the changes and inclusions for a custom ManifestClient not be required? My understanding is load_manifest in vite-land is always going to be looking for a .json file it is just a issue of where that file lives. ManifestClient would remain part of the library as long it gets pointed to the correct place to look via a setting instead of via inheritance. There is maybe similar arguments to be made with the two get_*_server_url but that might be overdoing it.

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.

4 participants