-
-
Notifications
You must be signed in to change notification settings - Fork 16
"fetch: lazy": don't download until the controller is present #15
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
'use strict'; | ||
/** | ||
* | ||
* @param {string} controllerPath The importable path to the controller | ||
* @param {Number} indentationSpaces Amount each line should be indented | ||
*/ | ||
|
||
module.exports = function generateLazyController(controllerPath, indentationSpaces) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we have dist files committed in this repo ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should clean this up. Some dist files are needed because they're read at runtime in the browser - specifically the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even for the necessary ones, I don't think they need to be committed, as the npm ecosystem relies on a separate publishing step and so the VCS content and the package content can be different. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right about that too. None of them should be committed. However, we would need to make sure there was a more sophisticated build+publish functionality. The dist files are also committed in symfony/ux for the same reason: ease of distribution (because there is no automated build/release procedure). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. symfony/ux packages are not distributed through npm but through composer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. npm already has a built-in automation: it will run the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This repo could definitely not include the dist files at all indeed. I added them without too much thoughts after having added them in symfony/ux. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's fix that in another PR |
||
var spaces = ' '.repeat(indentationSpaces); | ||
return "".concat(spaces, "(function() {\n").concat(spaces, " function LazyController(context) {\n").concat(spaces, " Controller.call(this, context);\n").concat(spaces, " }\n").concat(spaces, " LazyController.prototype = Object.create(Controller && Controller.prototype, {\n").concat(spaces, " constructor: { value: LazyController, writable: true, configurable: true }\n").concat(spaces, " });\n").concat(spaces, " Object.setPrototypeOf(LazyController, Controller);\n").concat(spaces, " LazyController.prototype.initialize = function() {\n").concat(spaces, " var _this = this;\n").concat(spaces, " import('").concat(controllerPath, "').then(function(controller) {\n").concat(spaces, " _this.application.register(_this.identifier, controller.default);\n").concat(spaces, " });\n").concat(spaces, " }\n").concat(spaces, " return LazyController;\n").concat(spaces, "})()"); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,13 @@ | |
|
||
'use strict'; | ||
|
||
const generateLazyController = require('./generate-lazy-controller'); | ||
|
||
module.exports = function createControllersModule(config) { | ||
let controllerContents = 'export default {'; | ||
let autoImportContents = ''; | ||
let hasLazyControllers = false; | ||
const deprecations = []; | ||
|
||
if ('undefined' !== typeof config['placeholder']) { | ||
throw new Error( | ||
|
@@ -28,6 +32,8 @@ module.exports = function createControllersModule(config) { | |
|
||
for (let controllerName in config.controllers[packageName]) { | ||
const controllerReference = packageName + '/' + controllerName; | ||
// Normalize the controller name: remove the initial @ and use Stimulus format | ||
const controllerNormalizedName = controllerReference.substr(1).replace(/_/g, '-').replace(/\//g, '--'); | ||
|
||
// Find package config for the controller | ||
if ('undefined' === typeof packageConfig.symfony.controllers[controllerName]) { | ||
|
@@ -44,16 +50,35 @@ module.exports = function createControllersModule(config) { | |
} | ||
|
||
const controllerMain = packageName + '/' + controllerPackageConfig.main; | ||
const webpackMode = controllerUserConfig.webpackMode; | ||
let fetchMode = 'eager'; | ||
|
||
if ('undefined' !== typeof controllerUserConfig.webpackMode) { | ||
deprecations.push( | ||
'The "webpackMode" config key is deprecated in controllers.json. Use "fetch" instead, set to either "eager" or "lazy".' | ||
); | ||
} | ||
|
||
if ('undefined' !== typeof controllerUserConfig.fetch) { | ||
if (!['eager', 'lazy'].includes(controllerUserConfig.fetch)) { | ||
throw new Error( | ||
`Invalid "fetch" value "${controllerUserConfig.fetch}" in controllers.json. Expected "eager" or "lazy".` | ||
); | ||
} | ||
|
||
controllerContents += | ||
"\n '" + | ||
controllerReference + | ||
'\': import(/* webpackMode: "' + | ||
webpackMode + | ||
'" */ \'' + | ||
controllerMain + | ||
"'),"; | ||
fetchMode = controllerUserConfig.fetch; | ||
} | ||
|
||
let moduleValueContents = `import(/* webpackMode: "eager" */ '${controllerMain}')`; | ||
if (fetchMode === 'lazy') { | ||
hasLazyControllers = true; | ||
moduleValueContents = ` | ||
new Promise((resolve, reject) => resolve({ default: | ||
weaverryan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
${generateLazyController(controllerMain, 6)} | ||
})) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because this runs after babel, we need to rewrite this into "old" JavaScript to support all browsers. I have the code locally, just need to put it in here :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or maybe we can make this run before babel There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be nice... I just could not find a way to do that :/. We effectively would need to change the "filename" of the output source to something that ends in |
||
`.trim(); | ||
} | ||
|
||
controllerContents += `\n '${controllerNormalizedName}': ${moduleValueContents},`; | ||
|
||
for (let autoimport in controllerUserConfig.autoimport || []) { | ||
if (controllerUserConfig.autoimport[autoimport]) { | ||
|
@@ -63,5 +88,12 @@ module.exports = function createControllersModule(config) { | |
} | ||
} | ||
|
||
return `${autoImportContents}${controllerContents}\n};`; | ||
if (hasLazyControllers) { | ||
controllerContents = `import { Controller } from 'stimulus';\n${controllerContents}`; | ||
} | ||
|
||
return { | ||
finalSource: `${autoImportContents}${controllerContents}\n};`, | ||
deprecations, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
/** | ||
* | ||
* @param {string} controllerPath The importable path to the controller | ||
* @param {Number} indentationSpaces Amount each line should be indented | ||
*/ | ||
module.exports = function generateLazyController(controllerPath, indentationSpaces) { | ||
const spaces = ' '.repeat(indentationSpaces); | ||
|
||
return `${spaces}(function() { | ||
${spaces} function LazyController(context) { | ||
${spaces} Controller.call(this, context); | ||
${spaces} } | ||
${spaces} LazyController.prototype = Object.create(Controller && Controller.prototype, { | ||
${spaces} constructor: { value: LazyController, writable: true, configurable: true } | ||
${spaces} }); | ||
${spaces} Object.setPrototypeOf(LazyController, Controller); | ||
${spaces} LazyController.prototype.initialize = function() { | ||
${spaces} var _this = this; | ||
${spaces} import('${controllerPath}').then(function(controller) { | ||
${spaces} _this.application.register(_this.identifier, controller.default); | ||
${spaces} }); | ||
${spaces} } | ||
${spaces} return LazyController; | ||
${spaces}})()`; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should open a PR on UX and Flex to change this there. Do you have time to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already did! :) symfony/ux#53
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But flex - I hadn’t thought about that piece - I’ll get that done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flex PR: symfony/flex#735