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 template tag to enable react-refresh #53

Merged
merged 3 commits into from
May 10, 2023

Conversation

BrandonShar
Copy link
Contributor

This PR adds a template tag called vite_react_refresh to handle providing the extra code needed to support react hot module reloading.

This was discussed a bit in #14 and #15 and while it is project specific and only relevant for react users, having this tag be part of the package is inline with other framework community adapters like Vite Rails and Vite Laravel

I wasn't sure how'd you like to format the multi-line string needed, but since it's dev only I figured the indenting didn't matter much. If you disagree, let me know and I'll revise it!

Thanks for this great repo!

@MrBin99
Copy link
Owner

MrBin99 commented Aug 21, 2022

Thanks for your PR but your code generates something for a specific frontend framework.

django-vite aims to be agnostic on the frontend framework you choose and if we start to implement every specificity for each frontend framework it will be very tedious.

I think django-vite must be very configurable to implement this by projects or we need, as discussed in issues, to have some sort of plugins or another system to let people opt-in to use specific stuff for React, Vue .... and also to be more configurable.

@BrandonShar
Copy link
Contributor Author

I’m a little confused, didn’t you say in #14 that you would accept a PR with this functionality?

if we start to implement every specificity for each frontend framework it will be very tedious.

Do you have an example of what you think this might lead to? I think react refresh is a special case because this is a known issue with Vite react that every other vite adapter I’ve seen handles. It’s more of a bug fix than an enhancement because it’s required for React to work with Vite.

@MrBin99
Copy link
Owner

MrBin99 commented Aug 21, 2022

Of course I will accept a PR, I didn't see that React is an official plugin of Vite so we could add it.
But I do not want to add to the project a lot of stuff like this for each frontend framework.
And I'm thinking a way to let the developer opt-in to use specific code for specific framework but there is no ideal way to do it.

So I think django-vite should support official plugins of Vite but that's all to not over complexify the lib.

Thanks for your PR, can you just make it more configurable and add some comments ? And I will merge your PR.

@BrandonShar
Copy link
Contributor Author

I'm happy to make changes, but I'm genuinely not sure what you're looking for. Both methods I added already have doc-blocks and there isn't much to configure. I added a setting for users to override the react-refresh url, but I don't even think that's necessary. Non-react users simply won't add the template tag, so there's nothing more that needs to be done there.

Let me know if there's something specific you're looking for!

Copy link
Collaborator

@thijskramer thijskramer left a comment

Choose a reason for hiding this comment

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

Looks good to me! @MrBin99 would you please take another look?

@AlessandroSalvetti
Copy link

Same for me, looks good. Could you @MrBin99 review it please?

@BrandonShar
Copy link
Contributor Author

I suspect this project has been abandoned. Since I maintain a project that's downstream of this, I think I'm going to fork it for a re-release. For those of you commenting here, is there anywhere (maybe discord?) that this project is discussed I could join?

@thijskramer
Copy link
Collaborator

A fork that keeps getting maintained would be very nice! If you need any help with the maintenance, I'm happy to do help with that.

I'm not sure if there is a space for discussions anywhere, maybe start a discussion in this repository? Or submit an issue with the proposal to possibly take over this project? Because it would be nice to keep the name 'django-vite'.

@AlessandroSalvetti
Copy link

I can help too.
If this project is abandoned and we need a fork, I think we should ask to Vite to be added in the official integration list (here) as this project is.

@thijskramer
Copy link
Collaborator

thijskramer commented May 9, 2023

Regarding #70 , @BrandonShar @AlessandroSalvetti and possibly others, would you be interested in a collaboration to revitalize this project?

@thijskramer
Copy link
Collaborator

thijskramer commented May 10, 2023

It still looks good to me so I'll merge this!

@thijskramer thijskramer merged commit eaef386 into MrBin99:master May 10, 2023
@BrandonShar BrandonShar deleted the add-react-refresh-tag branch May 10, 2023 19:29
blighj pushed a commit to blighj/django-vite that referenced this pull request May 14, 2023
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