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

Comment "stimulusFetch: 'lazy'" is dropped from Stimulus Controller #10

Closed
Kocal opened this issue Oct 24, 2024 · 15 comments · Fixed by symfony/ux#2304
Closed

Comment "stimulusFetch: 'lazy'" is dropped from Stimulus Controller #10

Kocal opened this issue Oct 24, 2024 · 15 comments · Fixed by symfony/ux#2304

Comments

@Kocal
Copy link

Kocal commented Oct 24, 2024

Hey :)

Given my Stimulus controller:

import { redo as commandRedo, undo as commandUndo } from "@codemirror/commands";
import { markdown } from "@codemirror/lang-markdown";
import { Compartment, EditorSelection } from "@codemirror/state";
import { githubDark } from "@fsegurai/codemirror-theme-github-dark";
import { githubLight } from "@fsegurai/codemirror-theme-github-light";
import { Controller } from "@hotwired/stimulus";
import { EditorView, basicSetup } from "codemirror";
import { codeLanguages } from "../codemirror/code_languages.js";
import { lightDarkThemeSwitcher } from "../codemirror/light_dark_theme_switcher.js";

/* stimulusFetch: 'lazy' */
export default class extends Controller {
    static targets = ["textarea"];
    static classes = ["editor"];

    /* ... */
}

After being minified, the comment /* stimulusFetch: 'lazy' */ disappear from the source code:
image

Which load the controller (even if not needed since there are no data-controller="markdown-editor" on the page) and load all the dependencies (a lot):
image

@Kocal
Copy link
Author

Kocal commented Oct 24, 2024

I've tried to add a ! to the comment, like this: /*! stimulusFetch: 'lazy' */, it's usually something that tell minifiers to NOT remove the comment, thus, Minify supports that behavior for CSS (https://github.com/tdewolff/minify#css) but for JS I don't know.

Anyway, if I use the new comment like that:

import { redo as commandRedo, undo as commandUndo } from "@codemirror/commands";
import { markdown } from "@codemirror/lang-markdown";
import { Compartment, EditorSelection } from "@codemirror/state";
import { githubDark } from "@fsegurai/codemirror-theme-github-dark";
import { githubLight } from "@fsegurai/codemirror-theme-github-light";
import { Controller } from "@hotwired/stimulus";
import { EditorView, basicSetup } from "codemirror";
import { codeLanguages } from "../codemirror/code_languages.js";
import { lightDarkThemeSwitcher } from "../codemirror/light_dark_theme_switcher.js";

/*! stimulusFetch: 'lazy' */
export default class extends Controller {
    static targets = ["textarea"];
    static classes = ["editor"];

Then I see the comment being at the very top of the file:
image

And the lazy-loading behavior still not works.

Kocal added a commit to Kocal/hugo.alliau.me that referenced this issue Oct 24, 2024
@smnandre
Copy link
Collaborator

Is it happening both for asset-map:compile and Minify:asset command or just the later?

@MatTheCat
Copy link

See tdewolff/minify#552

@MatTheCat
Copy link

MatTheCat commented Oct 26, 2024

Okay dug a little bit deeper and minify will only keep a comment if

  • it is “important”, meaning it starts with /*!
  • it is in top of the file (can be preceded by other important comments though)

So we could make this work by putting /*! stimulusFetch: 'lazy' */ on top of lazy controllers, and updating the StimulusBundle’s regexp so that it allows the exclamation mark…

@MatTheCat
Copy link

MatTheCat commented Oct 26, 2024

Just thought of something simpler 😅

StimulusBundle’s ControllersMapGenerator checks for the comment in the compiled asset: https://github.com/symfony/stimulus-bundle/blob/60ec83e56c94d441eae0c33d858addf6f1c780dd/src/AssetMapper/ControllersMapGenerator.php#L80

Always checking against the source file content would solve this issue:

- $content = $asset->content ?: file_get_contents($asset->sourcePath);
+ $content = file_get_contents($asset->sourcePath);

I feel like it would make sense 🤔

@smnandre
Copy link
Collaborator

Are you sure this does not work with AssetMapper ? When i tested it i’m pretty sure it did, but i may had another implémentation at that point

@smnandre
Copy link
Collaborator

(i’m 100km from my computer right now ^^)

@MatTheCat
Copy link

MatTheCat commented Oct 26, 2024

Yes I successfully reproduced what @Kocal reported: given the following controller:

import { Controller } from '@hotwired/stimulus';

/* stimulusFetch: 'lazy' */
export default class extends Controller {
    connect() {
        this.element.textContent = 'Hello Stimulus! Edit me in assets/controllers/hello_controller.js';
    }
}

The ControllersMapGenerator gets the following to search /* stimulusFetch: 'lazy' */ in:

import{Controller}from"@hotwired/stimulus";export default class extends Controller{connect(){this.element.textContent="Hello Stimulus! Edit me in assets/controllers/hello_controller.js"}}

It doesn’t find the comment and thus considers the controller to be loaded eagerly.

symfony/ux#2304 should fix this issue 🤞

@smnandre
Copy link
Collaborator

symfony/ux#2304 will be released in the incoming weeks, so not sure if we can do something in the meantime here :|

@Kocal
Copy link
Author

Kocal commented Oct 29, 2024

Sure, let's wait

@smnandre
Copy link
Collaborator

symfony/stimulus-bundle 2.2.22 has been released, with the fix from @MatTheCat

To me everything works as expected (had a weird cache first but not sure if related)

Could you guys check on your side?

@Kocal
Copy link
Author

Kocal commented Dec 1, 2024

I will check :)

@Kocal
Copy link
Author

Kocal commented Dec 1, 2024

... weeeell, after a whole day of VM/server issues (just after deploying the commit to use this bundle on my website.... 🧐 (just a coincidence don't worry 😛 )).

IDK if I did something wrong, but:

I believe this is an issue from AssetMapper and automatic usages of <link rel="modulepreload">, since you see the request initiator is the page itself and not Stimulus:
image

But if you take a look to controllers code, it is compressed as expected:
image

@MatTheCat
Copy link

MatTheCat commented Dec 2, 2024

Tried StimulusBundle 2.22 on a small project and it behaves as expected: controller is loaded when not being lazy or being used on the page.

@Kocal modulepreload probably means your places and markdown-editor controllers are imported from scripts reachable from your entry point 🤔
EDIT: indeed I can see they’re not considered lazy:

// controllers.js
import controller_0 from'../ux-turbo/turbo_controller.js';
import controller_1 from'../../controllers/places_controller.js';
import controller_2 from'../../controllers/markdown_editor_controller.js';
export const eagerControllers = {
  'symfony--ux-turbo--turbo-core': controller_0,
  places: controller_1,
  'markdown-editor': controller_2
};
export const lazyControllers = {
  'symfony--ux-leaflet-map--map': () => import ('../ux-leaflet-map/map_controller.js')
};
export const isApplicationDebug = !1

@Kocal
Copy link
Author

Kocal commented Dec 4, 2024

I'm stupid 🤦🏻

When upgrading Symfony UX packages, I ran symfony composer update 'symfony/ux-*', which... does not involve symfony/stimulus-bundle. I was still using StimulusBundle 2.21 🫠

Updated StimulusBundle to 2.22, it's working now!

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 a pull request may close this issue.

3 participants