Skip to content

[Bug] Removing unnecessary Promise in object of controllers to be loaded #81

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

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Mar 16, 2023

Hi!

Subtle change. Previously, the webpack loader parsed controllers.json (which eventually becomes the symfonyControllers variable in index.ts) into something that exported an object where each key was a promise - something like:

export default {
    'symfony--ux-autocomplete--autocomplete': import(/* webpackMode: \"eager\" */ '@symfony/ux-autocomplete/dist/controller.js')
}

Then, in startStimulusApp(), we called used .then() to wait for that promise to resolve then registered its resolved value:

symfonyControllers[controllerName].then((module) => {
    application.register(controllerName, module.default);
});

This is totally unnecessary and a relic of how this library was originally built. It also causes the UX controllers to be registered late. It's barely noticeable, but in practice, instead of the UX controllers being registered BEFORE the DOM is ready, they are registered after. This actually causes a problem with a new feature from LiveComponents.

The new parsed version of controllers.json from the loader looks much simpler:

+ import controller_0 from '@symfony/ux-autocomplete/dist/controller.js';

export default {
-    'symfony--ux-autocomplete--autocomplete': import(/* webpackMode: \"eager\" */ '@symfony/ux-autocomplete/dist/controller.js')
+    'symfony--ux-autocomplete--autocomplete': controller_0,
}

(I don't show it here, but lazy controllers have a similar simplification).

In short: it's a bug fix & an easy win. Controllers will load slightly earlier as a result.

Thanks!

@weaverryan weaverryan force-pushed the removing-unnecessary-promise branch from f7a10fd to 0d77e41 Compare March 20, 2023 19:22
@weaverryan weaverryan merged commit b49d87b into symfony:main Mar 20, 2023
weaverryan added a commit to symfony/ux that referenced this pull request Mar 22, 2023
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Site] Upgrading to latest version + other changes

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Tickets       | None
| License       | MIT

* Updating to latest version of UX (which will become 2.8.0)
* Upgrading deps
* Various small changes and fixes

TODO:
* [x] Use symfony/stimulus-bridge#81 when it's available

Commits
-------

6c9675b [Site] Upgrading to latest version + other changes
fullstackdeveloperwebapp added a commit to fullstackdeveloperwebapp/ux that referenced this pull request Aug 1, 2023
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Site] Upgrading to latest version + other changes

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Tickets       | None
| License       | MIT

* Updating to latest version of UX (which will become 2.8.0)
* Upgrading deps
* Various small changes and fixes

TODO:
* [x] Use symfony/stimulus-bridge#81 when it's available

Commits
-------

6c9675bf [Site] Upgrading to latest version + other changes
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.

2 participants