Skip to content

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