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

[StimulusBundle] Check controllers source files for laziness #2304

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Oct 26, 2024

Q A
Bug fix? no
New feature? yes
Issues Fix sensiolabs/minify-bundle#10
License MIT

The StimulusBundle allows to mark a controller as lazy using a /* stimulusFetch: 'lazy' */ comment, but it will be searched in said controller’s compiled content. If the compilation removes that comment (it typically happens when using minifiers), then the controller is no longer considered lazy by the ControllersMapGenerator.

This PR makes the comment searched in source files, so that its presence doesn’t depend on the compilation’s result.

Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Hi @MatTheCat, and thanks for the contribution.

I kind of agree with you, but I don't know how much of a breaking change it can be. :/

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Oct 26, 2024

I guess it would break compilers wanting to interact with the StimulusBundle by adding or removing the /* stimulusFetch: 'lazy' */ comment in controllers. Wouldn't it be better to decorate or replace the stimulus.asset_mapper.controllers_map_generator service in that case?

@smnandre
Copy link
Member

It feels to me a very internal matter... and a very logical change.

To make one of your custom controllers lazy, add a special comment on top: [...]

To me it's never an AssetMapper thing, but a source code thing, so..

I'd even call this a feature to be honest , as it would remove the following constraint:

Note

If you write your controllers using TypeScript, make sure removeComments is not set to true in your TypeScript config.

https://symfony.com/bundles/StimulusBundle/current/index.html#lazy-stimulus-controllers

So i'm clearly 👍 on this

@MatTheCat Do you think you could add a test or two ?

@MatTheCat MatTheCat force-pushed the stimulus_lazy_controller_check branch from 8b5aff6 to 82d828d Compare October 28, 2024 11:13
@MatTheCat
Copy link
Contributor Author

Test added!

@MatTheCat MatTheCat force-pushed the stimulus_lazy_controller_check branch from 82d828d to cd2e155 Compare October 28, 2024 11:18
Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

Thank you @MatTheCat ! Nice fix/feat :)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Oct 29, 2024
@Kocal
Copy link
Member

Kocal commented Oct 29, 2024

Thanks!

@javiereguiluz
Copy link
Member

Thanks for fixing this bug Mathieu.

@javiereguiluz javiereguiluz merged commit fffd7c8 into symfony:2.x Oct 30, 2024
59 checks passed
@MatTheCat MatTheCat deleted the stimulus_lazy_controller_check branch October 30, 2024 16:46
Kocal added a commit that referenced this pull request Nov 7, 2024
…nger be necessary (MatTheCat)

This PR was merged into the 2.x branch.

Discussion
----------

[StimulusBundle][Doc] Mention `removeComments` will no longer be necessary

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Issues        | N/A
| License       | MIT

Once #2304 is released, it will no longer be necessary to set `removeComments` to `false` when using TypeScript, because the `stimulusFetch: 'lazy'` comment will be searched in the source rather than the compiled content.

Commits
-------

a422be1 [StimulusBundle][Doc] Mention `removeComments` will no longer be necessary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix Status: Reviewed Has been reviewed by a maintainer StimulusBundle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comment "stimulusFetch: 'lazy'" is dropped from Stimulus Controller
5 participants