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

Allow Vite entry points to be merged #53233

Merged
merged 2 commits into from
Oct 21, 2024
Merged

Conversation

JackWH
Copy link
Contributor

@JackWH JackWH commented Oct 18, 2024

This PR aims to solve an issue when Vite entry points are conditionally determined at runtime.

The existing withEntryPoints method in the Vite class replaces any entry points which have already been applied, and $this->entryPoints is protected with no accessor method. So this backwards-compatible PR simply adds an additional method to merge a set of entry points with any which have already been set.


I recently found myself in this situation:

  • I have two main Vite entry points, app.css and app.js.
  • I also have some additional entry points for custom React components: component-1.ts, component-2.ts etc.
  • My app uses Filament for the backend admin functionality, so I've used ->viteTheme(['app.css', 'app.js']) to set my entry points in their service provider.
  • On pages which require a specific React component, I can add @vite(['component-2.ts']) to include them as needed.

However, if component-2.ts references assets which are also used by app.css (e.g. website logo etc.), duplicate chunk tags are rendered in the HTML markup.

With this PR I can let Filament call withEntryPoints in its service provider, then later attach any additional components I need to include conditionally, before letting Filament render them once in a single output.


There are other ways to solve this, I figured mergeEntryPoints was the simplest but am open to any other ideas.

Let me know if any changes are necessary. Thanks! 🙏

Copy link
Member

@timacdonald timacdonald left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

The additional tags output shouldn't cause unintended side effects, but I would probably be looking for another solution that felt right as well.

@taylorotwell taylorotwell merged commit f507e43 into laravel:11.x Oct 21, 2024
31 checks passed
@JackWH JackWH deleted the patch-2 branch October 21, 2024 14:49
@pkeogan
Copy link

pkeogan commented Oct 24, 2024

@JackWH Thank you for this PR, I came to the same conclusion you did after having the same issue. Glad to see your PR made it into v11.29.0

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