Skip to content

Commit b49d87b

Browse files
weaverryanweaverryan
committed
bug #81 [Bug] Removing unnecessary Promise in object of controllers to be loaded (weaverryan)
This PR was squashed before being merged into the main branch. Discussion ---------- [Bug] Removing unnecessary Promise in object of controllers to be loaded 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: ```js 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: ```js 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: ```diff + 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! Commits ------- 0d77e41 [Bug] Removing unnecessary Promise in object of controllers to be loaded
2 parents 7ce6105 + 0d77e41 commit b49d87b

11 files changed

+113
-166
lines changed

‎README.md‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ automatically when you install [Symfony UX Packages](https://github.com/symfony/
99
Before you start, familiarize yourself with the basics of
1010
[Stimulus](https://stimulus.hotwired.dev/).
1111

12-
Symfony UX Stimulus bridge is currently considered **experimental**.
13-
1412
## Installation
1513

1614
If you don't already have Webpack Encore installed, install it with:

‎dist/index.js‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,7 @@ function startStimulusApp(context) {
4141
if (!symfonyControllers.hasOwnProperty(controllerName)) {
4242
continue;
4343
}
44-
symfonyControllers[controllerName].then((module) => {
45-
application.register(controllerName, module.default);
46-
});
44+
application.register(controllerName, symfonyControllers[controllerName]);
4745
}
4846
return application;
4947
}

‎dist/webpack/lazy-controller-loader.js‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,24 +30,22 @@ var schemaUtils__namespace = /*#__PURE__*/_interopNamespace(schemaUtils);
3030

3131
function generateLazyController (controllerPath, indentationSpaces, exportName = 'default') {
3232
const spaces = ' '.repeat(indentationSpaces);
33-
return `${spaces}(function() {
34-
${spaces} return class LazyController extends Controller {
35-
${spaces} constructor(context) {
36-
${spaces} super(context);
37-
${spaces} this.__stimulusLazyController = true;
38-
${spaces} }
39-
${spaces} initialize() {
40-
${spaces} if (this.application.controllers.find((controller) => {
41-
${spaces} return controller.identifier === this.identifier && controller.__stimulusLazyController;
42-
${spaces} })) {
43-
${spaces} return;
44-
${spaces} }
45-
${spaces} import('${controllerPath.replace(/\\/g, '\\\\')}').then((controller) => {
46-
${spaces} this.application.register(this.identifier, controller.${exportName});
47-
${spaces} });
33+
return `class extends Controller {
34+
${spaces} constructor(context) {
35+
${spaces} super(context);
36+
${spaces} this.__stimulusLazyController = true;
37+
${spaces} }
38+
${spaces} initialize() {
39+
${spaces} if (this.application.controllers.find((controller) => {
40+
${spaces} return controller.identifier === this.identifier && controller.__stimulusLazyController;
41+
${spaces} })) {
42+
${spaces} return;
4843
${spaces} }
44+
${spaces} import('${controllerPath.replace(/\\/g, '\\\\')}').then((controller) => {
45+
${spaces} this.application.register(this.identifier, controller.${exportName});
46+
${spaces} });
4947
${spaces} }
50-
${spaces}})()`;
48+
${spaces}}`;
5149
}
5250

5351
// Reserved word lists for various dialects of the language

‎dist/webpack/loader.js‎

Lines changed: 30 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -8,37 +8,35 @@ var LoaderDependency__default = /*#__PURE__*/_interopDefaultLegacy(LoaderDepende
88

99
function generateLazyController (controllerPath, indentationSpaces, exportName = 'default') {
1010
const spaces = ' '.repeat(indentationSpaces);
11-
return `${spaces}(function() {
12-
${spaces} return class LazyController extends Controller {
13-
${spaces} constructor(context) {
14-
${spaces} super(context);
15-
${spaces} this.__stimulusLazyController = true;
16-
${spaces} }
17-
${spaces} initialize() {
18-
${spaces} if (this.application.controllers.find((controller) => {
19-
${spaces} return controller.identifier === this.identifier && controller.__stimulusLazyController;
20-
${spaces} })) {
21-
${spaces} return;
22-
${spaces} }
23-
${spaces} import('${controllerPath.replace(/\\/g, '\\\\')}').then((controller) => {
24-
${spaces} this.application.register(this.identifier, controller.${exportName});
25-
${spaces} });
11+
return `class extends Controller {
12+
${spaces} constructor(context) {
13+
${spaces} super(context);
14+
${spaces} this.__stimulusLazyController = true;
15+
${spaces} }
16+
${spaces} initialize() {
17+
${spaces} if (this.application.controllers.find((controller) => {
18+
${spaces} return controller.identifier === this.identifier && controller.__stimulusLazyController;
19+
${spaces} })) {
20+
${spaces} return;
2621
${spaces} }
22+
${spaces} import('${controllerPath.replace(/\\/g, '\\\\')}').then((controller) => {
23+
${spaces} this.application.register(this.identifier, controller.${exportName});
24+
${spaces} });
2725
${spaces} }
28-
${spaces}})()`;
26+
${spaces}}`;
2927
}
3028

3129
function createControllersModule(config) {
3230
let controllerContents = 'export default {';
33-
let autoImportContents = '';
31+
let importStatementContents = '';
3432
let hasLazyControllers = false;
35-
const deprecations = [];
3633
if ('undefined' !== typeof config['placeholder']) {
3734
throw new Error('Your controllers.json file was not found. Be sure to add a Webpack alias from "@symfony/stimulus-bridge/controllers.json" to *your* controllers.json file.');
3835
}
3936
if ('undefined' === typeof config['controllers']) {
4037
throw new Error('Your Stimulus configuration file (assets/controllers.json) lacks a "controllers" key.');
4138
}
39+
let controllerIndex = 0;
4240
for (const packageName in config.controllers) {
4341
let packageConfig;
4442
try {
@@ -58,24 +56,19 @@ function createControllersModule(config) {
5856
continue;
5957
}
6058
const controllerMain = packageName + '/' + controllerPackageConfig.main;
61-
let fetchMode = 'eager';
62-
if ('undefined' !== typeof controllerUserConfig.webpackMode) {
63-
deprecations.push('The "webpackMode" config key is deprecated in controllers.json. Use "fetch" instead, set to either "eager" or "lazy".');
59+
let fetchMode = controllerUserConfig.fetch || 'eager';
60+
let moduleValueContents = ``;
61+
if (fetchMode === 'eager') {
62+
const controllerNameForVariable = `controller_${controllerIndex++}`;
63+
importStatementContents += `import ${controllerNameForVariable} from '${controllerMain}';\n`;
64+
moduleValueContents = controllerNameForVariable;
6465
}
65-
if ('undefined' !== typeof controllerUserConfig.fetch) {
66-
if (!['eager', 'lazy'].includes(controllerUserConfig.fetch)) {
67-
throw new Error(`Invalid "fetch" value "${controllerUserConfig.fetch}" in controllers.json. Expected "eager" or "lazy".`);
68-
}
69-
fetchMode = controllerUserConfig.fetch;
70-
}
71-
let moduleValueContents = `import(/* webpackMode: "eager" */ '${controllerMain}')`;
72-
if (fetchMode === 'lazy') {
66+
else if (fetchMode === 'lazy') {
7367
hasLazyControllers = true;
74-
moduleValueContents = `
75-
new Promise((resolve, reject) => resolve({ default:
76-
${generateLazyController(controllerMain, 6)}
77-
}))
78-
`.trim();
68+
moduleValueContents = generateLazyController(controllerMain, 2);
69+
}
70+
else {
71+
throw new Error(`Invalid fetch mode "${fetchMode}" in controllers.json. Expected "eager" or "lazy".`);
7972
}
8073
let controllerNormalizedName = controllerReference.substr(1).replace(/_/g, '-').replace(/\//g, '--');
8174
if ('undefined' !== typeof controllerPackageConfig.name) {
@@ -87,7 +80,7 @@ ${generateLazyController(controllerMain, 6)}
8780
controllerContents += `\n '${controllerNormalizedName}': ${moduleValueContents},`;
8881
for (const autoimport in controllerUserConfig.autoimport || []) {
8982
if (controllerUserConfig.autoimport[autoimport]) {
90-
autoImportContents += "import '" + autoimport + "';\n";
83+
importStatementContents += "import '" + autoimport + "';\n";
9184
}
9285
}
9386
}
@@ -96,8 +89,8 @@ ${generateLazyController(controllerMain, 6)}
9689
controllerContents = `import { Controller } from '@hotwired/stimulus';\n${controllerContents}`;
9790
}
9891
return {
99-
finalSource: `${autoImportContents}${controllerContents}\n};`,
100-
deprecations,
92+
finalSource: `${importStatementContents}${controllerContents}\n};`,
93+
deprecations: [],
10194
};
10295
}
10396

‎src/index.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,7 @@ export function startStimulusApp(context: __WebpackModuleApi.RequireContext) {
3434
continue;
3535
}
3636

37-
symfonyControllers[controllerName].then((module: any) => {
38-
application.register(controllerName, module.default);
39-
});
37+
application.register(controllerName, symfonyControllers[controllerName]);
4038
}
4139

4240
return application;

‎src/webpack/create-controllers-module.ts‎

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,8 @@ import generateLazyController from './generate-lazy-controller';
1313

1414
export default function createControllersModule(config: any) {
1515
let controllerContents = 'export default {';
16-
let autoImportContents = '';
16+
let importStatementContents = '';
1717
let hasLazyControllers = false;
18-
const deprecations = [];
1918

2019
if ('undefined' !== typeof config['placeholder']) {
2120
throw new Error(
@@ -27,6 +26,7 @@ export default function createControllersModule(config: any) {
2726
throw new Error('Your Stimulus configuration file (assets/controllers.json) lacks a "controllers" key.');
2827
}
2928

29+
let controllerIndex = 0;
3030
for (const packageName in config.controllers) {
3131
let packageConfig;
3232
try {
@@ -55,32 +55,20 @@ export default function createControllersModule(config: any) {
5555
}
5656

5757
const controllerMain = packageName + '/' + controllerPackageConfig.main;
58-
let fetchMode = 'eager';
58+
let fetchMode = controllerUserConfig.fetch || 'eager';
5959

60-
if ('undefined' !== typeof controllerUserConfig.webpackMode) {
61-
deprecations.push(
62-
'The "webpackMode" config key is deprecated in controllers.json. Use "fetch" instead, set to either "eager" or "lazy".'
63-
);
64-
}
65-
66-
if ('undefined' !== typeof controllerUserConfig.fetch) {
67-
if (!['eager', 'lazy'].includes(controllerUserConfig.fetch)) {
68-
throw new Error(
69-
`Invalid "fetch" value "${controllerUserConfig.fetch}" in controllers.json. Expected "eager" or "lazy".`
70-
);
71-
}
72-
73-
fetchMode = controllerUserConfig.fetch;
74-
}
60+
let moduleValueContents = ``;
61+
if (fetchMode === 'eager') {
62+
const controllerNameForVariable = `controller_${controllerIndex++}`;
63+
importStatementContents += `import ${controllerNameForVariable} from '${controllerMain}';\n`;
7564

76-
let moduleValueContents = `import(/* webpackMode: "eager" */ '${controllerMain}')`;
77-
if (fetchMode === 'lazy') {
65+
// simply use the imported controller
66+
moduleValueContents = controllerNameForVariable;
67+
} else if (fetchMode === 'lazy') {
7868
hasLazyControllers = true;
79-
moduleValueContents = `
80-
new Promise((resolve, reject) => resolve({ default:
81-
${generateLazyController(controllerMain, 6)}
82-
}))
83-
`.trim();
69+
moduleValueContents = generateLazyController(controllerMain, 2);
70+
} else {
71+
throw new Error(`Invalid fetch mode "${fetchMode}" in controllers.json. Expected "eager" or "lazy".`);
8472
}
8573

8674
// Normalize the controller name: remove the initial @ and use Stimulus format
@@ -97,7 +85,7 @@ ${generateLazyController(controllerMain, 6)}
9785

9886
for (const autoimport in controllerUserConfig.autoimport || []) {
9987
if (controllerUserConfig.autoimport[autoimport]) {
100-
autoImportContents += "import '" + autoimport + "';\n";
88+
importStatementContents += "import '" + autoimport + "';\n";
10189
}
10290
}
10391
}
@@ -108,7 +96,7 @@ ${generateLazyController(controllerMain, 6)}
10896
}
10997

11098
return {
111-
finalSource: `${autoImportContents}${controllerContents}\n};`,
112-
deprecations,
99+
finalSource: `${importStatementContents}${controllerContents}\n};`,
100+
deprecations: [],
113101
};
114102
}

‎src/webpack/generate-lazy-controller.ts‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,22 +18,20 @@
1818
export default function(controllerPath: string, indentationSpaces: number, exportName = 'default') {
1919
const spaces = ' '.repeat(indentationSpaces);
2020

21-
return `${spaces}(function() {
22-
${spaces} return class LazyController extends Controller {
23-
${spaces} constructor(context) {
24-
${spaces} super(context);
25-
${spaces} this.__stimulusLazyController = true;
26-
${spaces} }
27-
${spaces} initialize() {
28-
${spaces} if (this.application.controllers.find((controller) => {
29-
${spaces} return controller.identifier === this.identifier && controller.__stimulusLazyController;
30-
${spaces} })) {
31-
${spaces} return;
32-
${spaces} }
33-
${spaces} import('${controllerPath.replace(/\\/g, '\\\\')}').then((controller) => {
34-
${spaces} this.application.register(this.identifier, controller.${exportName});
35-
${spaces} });
21+
return `class extends Controller {
22+
${spaces} constructor(context) {
23+
${spaces} super(context);
24+
${spaces} this.__stimulusLazyController = true;
25+
${spaces} }
26+
${spaces} initialize() {
27+
${spaces} if (this.application.controllers.find((controller) => {
28+
${spaces} return controller.identifier === this.identifier && controller.__stimulusLazyController;
29+
${spaces} })) {
30+
${spaces} return;
3631
${spaces} }
32+
${spaces} import('${controllerPath.replace(/\\/g, '\\\\')}').then((controller) => {
33+
${spaces} this.application.register(this.identifier, controller.${exportName});
34+
${spaces} });
3735
${spaces} }
38-
${spaces}})()`;
36+
${spaces}}`;
3937
}

‎test/fixtures/deprecated-webpack-mode.json‎

Lines changed: 0 additions & 11 deletions
This file was deleted.

0 commit comments

Comments
 (0)