TL;DR: The current addModuleStyles() method no longer meets our requirements. This mismatch causes bugs (e.g. user styles load twice). The current logic also doesn't support dynamic modules depending on style modules. I'm looking for feedback on how to best address these issues.
ResourceLoader is designed with two module types: Page style modules and dynamic modules.
Page style modules are part of the critical rendering path and should work without JavaScript being enabled or supported. Their URL must be referenced directly in the HTML to allow browsers to discover them statically for optimal performance. As such, this URL can't use dynamic information from the startup module (no dependencies[1], no version hashes).
Dynamic modules have their names listed in the page HTML. Their dependencies and version hashes are resolved at run-time by JavaScript via the startup manifest. We then generate the urls and request them from the server. There is also a layer of object caching in-between (localStorage) which often optimises the module request to not involve an HTTP request at all.
Below I explain the two issues, followed by a proposed solution.
## Dependency
Historically there was no overlap between these two kinds of modules. Page style modules were mostly dominated by the skin and apply to skin-specific server-generated html. Dynamic modules (skin agnostic) would make their own DOM and apply their own styles.
Now that we're reusing styles more, we're starting seeing to see overlap. In the past we used jQuery UI - its styles never applied to PHP-generated output. Now, with OOUI, its styles do apply to both server-side and client-side generated elements. Its styles are preloaded as a page style module on pages that contain OOUI widgets.
The OOjs UI bundle (and its additional styles) shouldn't need to know whether the current page already got some of the relevant styles. This is what dependencies are for. For OOUI specifically, we currently have a hardcoded work around that adds a hidden marker to pages where OOUI is preloaded. The OOUI style module has a skipFunction that forgoes loading if the marker is present.
## Implicit type
Aside from the need for dependencies between dynamic and page style modules. There is another bug related to this. We don't currently require modules to say whether they are a dynamic module or a page style module. In most cases this doesn't matter since a developer would only load it one way and the module type is implied. For example, if you only ever pass a module to addModules() or mw.loader() it is effectively a dynamic module. If you only ever pass a module to addModuleStyles() loaded via a <link rel=stylesheet> then it is a page style module.
A problem happens if one tries to load the same module both ways. This might seem odd (and it is), but happens unintentionally right now with wiki modules (specifically, user/site modules and gadgets).
For user/site modules, we don't know whether common.css relates to the page content, or whether it relates to dynamic content produced by common.js. The same for gadgets. A gadget may produce an AJAX interface and register styles for it, or the gadget may be styles-only and intended as a skin customisation. Right now the Gadgets extension works around this problem with a compromise: Load it both ways. First it loads all gadgets as page style modules (ignoring the script portion), and then it loads the same modules again as dynamic modules. Thus loading the styles twice.
## Proposed solution
In order to allow dependency relationships between a dynamic module and a page style module, we need to inform mw.loader in JavaScript about which modules have already been loaded by different means. We do this already with the user modules (by setting mw.loader.state directly).
This would work but means that if you then load the same module again as dynamic module, it will assume the module is already loaded and thus never deliver the script portion (for the case where the gadget wasn't meant to be a page style module). Similarly, it would mean that common.js wouldn't get delivered if common.css exists.
For user/site modules I propose to solve this by splitting them up into 'user', 'user.styles', 'site' and 'site.styles'. Existing dependency relationships between other modules and 'user' or 'site' will continue to work. It'd mostly be an internal detail. This allows us to load one as a page style module and the other dynamically.
For gadgets we can try to infer the intent (styles-only = page style module, both = dynamic module), with perhaps a way to declare the desired load method explicitly in Gadgets-definition if the default is wrong.
With that resolved, we can export mw.loader.state information for page style modules, which then allows dynamic modules to depend on them.
Thoughts?
https://phabricator.wikimedia.org/T87871 https://phabricator.wikimedia.org/T92459
-- Krinkle
[1] If one would allow page style modules to have dependencies and resolve them server-side in the HTML output, this would cause corruption when the relationship between two modules changes as existing pages would have the old relationship cached but do get the latest content from the server. Adding versions wouldn't help since the server can't feasibly have access to previous versions (too many page/skin/language combinations).
Quick notes on the migration path:
*Cached page HTML*
HTML cached under the old regime would still be served for a while, but ResourceLoader would load the newer style. I *think* that should work as long as the new versions of the modules that were included before still list the style-only modules as dependencies, assuming load.php doesn't throw an exception when the code-plus-style modules are requested.
If load.php would throw an exception in this case, then that's going to be a tricky problem to figure out for end-users, who will be presented with unstyled pages and no visible error message.
*Extension authors and users of extensions*
If addModuleStyles() throws a PHP exception at page-output time for modules that include scripts, that's a breaking change for extension authors and the people who use their extensions; but at least the error message will be very direct and immediate.
It'll need to be called out in the release notes with a section about breaking upgrade changes.
Ideally, the resulting error message that is displayed should also be very clear and include a link to a documentation page explaining how extension authors can update their code appropriately.
-- brion
On Wed, May 4, 2016 at 3:34 AM, Krinkle krinklemail@gmail.com wrote:
TL;DR: The current addModuleStyles() method no longer meets our requirements. This mismatch causes bugs (e.g. user styles load twice). The current logic also doesn't support dynamic modules depending on style modules. I'm looking for feedback on how to best address these issues.
ResourceLoader is designed with two module types: Page style modules and dynamic modules.
Page style modules are part of the critical rendering path and should work without JavaScript being enabled or supported. Their URL must be referenced directly in the HTML to allow browsers to discover them statically for optimal performance. As such, this URL can't use dynamic information from the startup module (no dependencies[1], no version hashes).
Dynamic modules have their names listed in the page HTML. Their dependencies and version hashes are resolved at run-time by JavaScript via the startup manifest. We then generate the urls and request them from the server. There is also a layer of object caching in-between (localStorage) which often optimises the module request to not involve an HTTP request at all.
Below I explain the two issues, followed by a proposed solution.
## Dependency
Historically there was no overlap between these two kinds of modules. Page style modules were mostly dominated by the skin and apply to skin-specific server-generated html. Dynamic modules (skin agnostic) would make their own DOM and apply their own styles.
Now that we're reusing styles more, we're starting seeing to see overlap. In the past we used jQuery UI - its styles never applied to PHP-generated output. Now, with OOUI, its styles do apply to both server-side and client-side generated elements. Its styles are preloaded as a page style module on pages that contain OOUI widgets.
The OOjs UI bundle (and its additional styles) shouldn't need to know whether the current page already got some of the relevant styles. This is what dependencies are for. For OOUI specifically, we currently have a hardcoded work around that adds a hidden marker to pages where OOUI is preloaded. The OOUI style module has a skipFunction that forgoes loading if the marker is present.
## Implicit type
Aside from the need for dependencies between dynamic and page style modules. There is another bug related to this. We don't currently require modules to say whether they are a dynamic module or a page style module. In most cases this doesn't matter since a developer would only load it one way and the module type is implied. For example, if you only ever pass a module to addModules() or mw.loader() it is effectively a dynamic module. If you only ever pass a module to addModuleStyles() loaded via a <link rel=stylesheet> then it is a page style module.
A problem happens if one tries to load the same module both ways. This might seem odd (and it is), but happens unintentionally right now with wiki modules (specifically, user/site modules and gadgets).
For user/site modules, we don't know whether common.css relates to the page content, or whether it relates to dynamic content produced by common.js. The same for gadgets. A gadget may produce an AJAX interface and register styles for it, or the gadget may be styles-only and intended as a skin customisation. Right now the Gadgets extension works around this problem with a compromise: Load it both ways. First it loads all gadgets as page style modules (ignoring the script portion), and then it loads the same modules again as dynamic modules. Thus loading the styles twice.
## Proposed solution
In order to allow dependency relationships between a dynamic module and a page style module, we need to inform mw.loader in JavaScript about which modules have already been loaded by different means. We do this already with the user modules (by setting mw.loader.state directly).
This would work but means that if you then load the same module again as dynamic module, it will assume the module is already loaded and thus never deliver the script portion (for the case where the gadget wasn't meant to be a page style module). Similarly, it would mean that common.js wouldn't get delivered if common.css exists.
For user/site modules I propose to solve this by splitting them up into 'user', 'user.styles', 'site' and 'site.styles'. Existing dependency relationships between other modules and 'user' or 'site' will continue to work. It'd mostly be an internal detail. This allows us to load one as a page style module and the other dynamically.
For gadgets we can try to infer the intent (styles-only = page style module, both = dynamic module), with perhaps a way to declare the desired load method explicitly in Gadgets-definition if the default is wrong.
With that resolved, we can export mw.loader.state information for page style modules, which then allows dynamic modules to depend on them.
Thoughts?
https://phabricator.wikimedia.org/T87871 https://phabricator.wikimedia.org/T92459
-- Krinkle
[1] If one would allow page style modules to have dependencies and resolve them server-side in the HTML output, this would cause corruption when the relationship between two modules changes as existing pages would have the old relationship cached but do get the latest content from the server. Adding versions wouldn't help since the server can't feasibly have access to previous versions (too many page/skin/language combinations). _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, May 4, 2016 at 1:43 PM, Brion Vibber bvibber@wikimedia.org wrote:
Quick notes on the migration path:
*Cached page HTML*
HTML cached under the old regime would still be served for a while, but ResourceLoader would load the newer style. I *think* that should work as long as the new versions of the modules that were included before still list the style-only modules as dependencies, assuming load.php doesn't throw an exception when the code-plus-style modules are requested.
If load.php would throw an exception in this case, then that's going to be a tricky problem to figure out for end-users, who will be presented with unstyled pages and no visible error message.
The restriction added for T92459 only applies to addModuleStyles() calls. No changes to load.php HTTP response behaviour.
There is one change required before we enable the new restriction: Convert internal details of how user/site/gadget modules are registered and loaded. These will be forward and backward compatible.
The new rule doesn't enable conceptually new features. We are currently avoiding a certain kind of relationship for performance reasons (dynamic modules depending on style modules) - which we won't need to avoid any longer. We'll need to wait for cache to roll over in between two steps to avoid styles from going missing.
Currently: * Output A: <link modules=site> (loads site styles), mw.loader.load('site') - (loads site scripts and styles)
Step 1: * Create 'site.styles' * Output A behaviour unchanged * New Output B: <link modules=site.styles>, site.styles=ready, mw.loader.load('site') - (loads site scripts and styles)
Step 2: * Remove styles from 'site', make 'site' depend on 'site.styles' (only affects startup module) * No change in HTML output * Output B new behaviour: <link modules=site.styles>, site.styles=ready, mw.loader.load('site') - (loads site scripts)
On Wed, May 4, 2016 at 1:43 PM, Brion Vibber bvibber@wikimedia.org wrote:
*Extension authors and users of extensions*
If addModuleStyles() throws a PHP exception at page-output time for modules that include scripts, that's a breaking change for extension authors and the people who use their extensions; but at least the error message will be very direct and immediate.
Yeah, we'll provide good release notes upfront. And probably 1 or 2 week iteration in git-master with warning only before enforcing with a run-time exception.
-- Timo
On Monday, May 9, 2016, Krinkle krinklemail@gmail.com wrote:
On Wed, May 4, 2016 at 1:43 PM, Brion Vibber <bvibber@wikimedia.org javascript:;> wrote:
Quick notes on the migration path:
*Cached page HTML*
HTML cached under the old regime would still be served for a while, but ResourceLoader would load the newer style. I *think* that should work as long as the new versions of the modules that were included before still list the style-only modules as dependencies, assuming load.php doesn't throw an exception when the code-plus-style modules are requested.
If load.php would throw an exception in this case, then that's going to
be
a tricky problem to figure out for end-users, who will be presented with unstyled pages and no visible error message.
The restriction added for T92459 only applies to addModuleStyles() calls. No changes to load.php HTTP response behaviour.
There is one change required before we enable the new restriction: Convert internal details of how user/site/gadget modules are registered and loaded. These will be forward and backward compatible.
The new rule doesn't enable conceptually new features. We are currently avoiding a certain kind of relationship for performance reasons (dynamic modules depending on style modules) - which we won't need to avoid any longer. We'll need to wait for cache to roll over in between two steps to avoid styles from going missing.
Currently:
- Output A: <link modules=site> (loads site styles), mw.loader.load('site')
- (loads site scripts and styles)
Step 1:
- Create 'site.styles'
- Output A behaviour unchanged
- New Output B: <link modules=site.styles>, site.styles=ready,
mw.loader.load('site') - (loads site scripts and styles)
Step 2:
- Remove styles from 'site', make 'site' depend on 'site.styles' (only
affects startup module)
- No change in HTML output
- Output B new behaviour: <link modules=site.styles>, site.styles=ready,
mw.loader.load('site') - (loads site scripts)
So, step 2 happens at least 30 days after deploying step 1? Or step 2 retains same behavior for Output A?
-- brion
On Wed, May 4, 2016 at 1:43 PM, Brion Vibber <bvibber@wikimedia.org javascript:;> wrote:
*Extension authors and users of extensions*
If addModuleStyles() throws a PHP exception at page-output time for
modules
that include scripts, that's a breaking change for extension authors and the people who use their extensions; but at least the error message will
be
very direct and immediate.
Yeah, we'll provide good release notes upfront. And probably 1 or 2 week iteration in git-master with warning only before enforcing with a run-time exception.
-- Timo _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org javascript:; https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Mon, May 9, 2016 at 7:17 PM, Brion Vibber bvibber@wikimedia.org wrote:
So, step 2 happens at least 30 days after deploying step 1? Or step 2 retains same behavior for Output A?
-- brion
14 days [1]. But yes, we'll need some breathing room after step 1.
After step 2, old output A would try to load "site" as style module still (which no longer has styles). Though after a FOUC, it would still fallback by loading 'site.styles' on-demand as dependency of "site".
[1] https://phabricator.wikimedia.org/T124954
-- Krinkle
Next steps now written up at https://phabricator.wikimedia.org/T92459#2281878
On Mon, May 9, 2016 at 7:45 PM, Krinkle krinklemail@gmail.com wrote:
On Mon, May 9, 2016 at 7:17 PM, Brion Vibber bvibber@wikimedia.org wrote:
So, step 2 happens at least 30 days after deploying step 1? Or step 2 retains same behavior for Output A?
-- brion
14 days [1]. But yes, we'll need some breathing room after step 1.
After step 2, old output A would try to load "site" as style module still (which no longer has styles). Though after a FOUC, it would still fallback by loading 'site.styles' on-demand as dependency of "site".
[1] https://phabricator.wikimedia.org/T124954
-- Krinkle
On Wed, May 4, 2016 at 6:34 AM, Krinkle krinklemail@gmail.com wrote:
## Implicit type
Aside from the need for dependencies between dynamic and page style modules. There is another bug related to this. We don't currently require modules to say whether they are a dynamic module or a page style module.
Maybe we *should* require that.
Then that could allow us to resolve one of my pet peeves with ResourceLoader: we can remove the need for general developers to know which modules need addModules() and which need addModuleStyles(). They can use addModules() for all modules and RL can just do the right thing based on the information provided by the creator of the module who is more likely to actually know whether something needs to be in <link> or JS-loaded.
Although for maximum benefit there, we would also have to make RL resolve dependencies to collect all the page style modules on the server side (more on that below). This would also be an improvement for general developers, as they would no longer need to know that the module they want to use has some dependency that needs to have addModuleStyles() manually called.
For gadgets we can try to infer the intent (styles-only = page style module, both = dynamic module), with perhaps a way to declare the desired load method explicitly in Gadgets-definition if the default is wrong.
A gadget might want both: a handful of "page" styles to prevent FoUC, and a much larger set of "dynamic" styles that can be loaded asynchronously along with the JS so as to not delay the rest of the page.
[1] If one would allow page style modules to have dependencies and resolve them server-side in the HTML output, this would cause corruption when the relationship between two modules changes as existing pages would have the old relationship cached but do get the latest content from the server. Adding versions wouldn't help since the server can't feasibly have access to previous versions (too many page/skin/language combinations).
But don't we have the corruption anyway? Say page Foo has a page style module 'foo', so it calls addModuleStyles( [ 'foo' ] ). Then module 'foo' is changed so it also needs 'bar', so page Foo now has to call addModuleStyles( [ 'foo', 'bar' ] ). What is avoided there that isn't avoided when addModuleStyles( [ 'foo' ] ) is smart enough to internally see that 'foo' depends on 'bar' and act as if it were passed [ 'foo', 'bar' ]? Or what case am I missing?
On the other hand, dependencies avoid the case where the developer modifying 'foo' doesn't realize that he has to search for everything everywhere that passes 'foo' to addModuleStyles() and manually add 'bar' to each one.
On Wed, May 4, 2016 at 8:23 AM, Brad Jorsch (Anomie) bjorsch@wikimedia.org wrote:
On Wed, May 4, 2016 at 6:34 AM, Krinkle krinklemail@gmail.com wrote:
[1] If one would allow page style modules to have dependencies and
resolve
them server-side in the HTML output, this would cause corruption when the relationship between two modules changes as existing pages would have the old relationship cached but do get the latest content from the server. Adding versions wouldn't help since the server can't feasibly have access to previous versions (too many page/skin/language combinations).
But don't we have the corruption anyway? Say page Foo has a page style module 'foo', so it calls addModuleStyles( [ 'foo' ] ). Then module 'foo' is changed so it also needs 'bar', so page Foo now has to call addModuleStyles( [ 'foo', 'bar' ] ). What is avoided there that isn't avoided when addModuleStyles( [ 'foo' ] ) is smart enough to internally see that 'foo' depends on 'bar' and act as if it were passed [ 'foo', 'bar' ]? Or what case am I missing?
On the other hand, dependencies avoid the case where the developer modifying 'foo' doesn't realize that he has to search for everything everywhere that passes 'foo' to addModuleStyles() and manually add 'bar' to each one.
Hmmmm... wait, does load.php not resolve dependencies server-side when producing concatenated CSS output? That would seem to break the transitional model I proposed if so.
Ah, fun. :)
-- brion
Apologies if I'm missing something that makes this so complicated but could we not simply throw an error/warning if you use addModuleStyles on a module with scripts set and leave this problem to the engineer to solve?
e.g. force 'a.lib' => [ 'styles' => [ 'a.css' ] 'scripts' => ['a.js'] ]
to become 'a.styles' => [ 'styles' => [ 'a.css' ] ] 'a.lib' => [ 'dependencies' => ['a.styles'], 'scripts' => ['a.js'] ]
This will obviously require lots of fixes in extensions using the old method but we've been here before when we enforced style modules having to set a position.
On Wed, May 4, 2016 at 9:44 AM, Brion Vibber bvibber@wikimedia.org wrote:
On Wed, May 4, 2016 at 8:23 AM, Brad Jorsch (Anomie) bjorsch@wikimedia.org wrote:
On Wed, May 4, 2016 at 6:34 AM, Krinkle krinklemail@gmail.com wrote:
[1] If one would allow page style modules to have dependencies and
resolve
them server-side in the HTML output, this would cause corruption when the relationship between two modules changes as existing pages would have the old relationship cached but do get the latest content from the server. Adding versions wouldn't help since the server can't feasibly have access to previous versions (too many page/skin/language combinations).
But don't we have the corruption anyway? Say page Foo has a page style module 'foo', so it calls addModuleStyles( [ 'foo' ] ). Then module 'foo' is changed so it also needs 'bar', so page Foo now has to call addModuleStyles( [ 'foo', 'bar' ] ). What is avoided there that isn't avoided when addModuleStyles( [ 'foo' ] ) is smart enough to internally see that 'foo' depends on 'bar' and act as if it were passed [ 'foo', 'bar' ]? Or what case am I missing?
On the other hand, dependencies avoid the case where the developer modifying 'foo' doesn't realize that he has to search for everything everywhere that passes 'foo' to addModuleStyles() and manually add 'bar' to each one.
Hmmmm... wait, does load.php not resolve dependencies server-side when producing concatenated CSS output? That would seem to break the transitional model I proposed if so.
Ah, fun. :)
-- brion _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Sun, May 8, 2016 at 5:47 PM, Jon Robson jrobson@wikimedia.org wrote:
Apologies if I'm missing something that makes this so complicated but could we not simply throw an error/warning if you use addModuleStyles on a module with scripts set
That's exactly what I'm proposing we do. https://phabricator.wikimedia.org/T92459
-- Timo
Okay great! :) I got lost in the details. Lets make it so!
On Mon, May 9, 2016 at 6:50 AM, Krinkle krinklemail@gmail.com wrote:
On Sun, May 8, 2016 at 5:47 PM, Jon Robson jrobson@wikimedia.org wrote:
Apologies if I'm missing something that makes this so complicated but could we not simply throw an error/warning if you use addModuleStyles on a module with scripts set
That's exactly what I'm proposing we do. https://phabricator.wikimedia.org/T92459
-- Timo _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, May 4, 2016 at 4:23 PM, Brad Jorsch (Anomie) bjorsch@wikimedia.org wrote:
On Wed, May 4, 2016 at 6:34 AM, Krinkle krinklemail@gmail.com wrote:
[..] We don't currently require modules to say whether they are a
dynamic module.
Maybe we *should* require that.
Then that could allow us to [remove] the need for general developers to know which modules need addModules() and which need addModuleStyles(). They can use addModules() for all modules and RL can just do the right thing [..]
Agreed. Though I'd like to tackle that separately in https://phabricator.wikimedia.org/T127328. See the section about removing "position" and adding a module type property.
On Wed, May 4, 2016 at 4:23 PM, Brad Jorsch (Anomie) bjorsch@wikimedia.org wrote:
On Wed, May 4, 2016 at 6:34 AM, Krinkle krinklemail@gmail.com wrote:
For gadgets we can try to infer the intent [with] a way to [declare]
explicitly.
A gadget might want both [..]
I acknowledge this use case. We are no longer allowing hybrid modules (which were never intentionally allowed, and don't properly work as demonstrated by the styles loading twice), but the idea of providing both is valid.
In such case one merely needs to register them separately and declare a dependency. Similar to what we'll do for the user/site modules, and like we'll do for oojs-ui.
Since the styles portion may not be useful for end-users directly, such module can be registered as hidden gadget. This concept is relatively new, but should work fine for now. (Back-ported from Gadgets 2.0.)
On Wed, May 4, 2016 at 4:23 PM, Brad Jorsch (Anomie) bjorsch@wikimedia.org wrote:
[..] But don't we have the corruption anyway? [..] module 'foo'
is changed so it also needs 'bar', so page Foo now has to call
addModuleStyles( [ 'foo', 'bar' ] ). [..]
The corruption I hypothesised is avoided because by design, page styles modules may not have dependencies. If a module 'foo' ignores that design requirement and changes regardless to require 'bar', then that change is in error and should be reverted.
There is no one way to make everything easy. In designing RL we saw two ways to make style modules work, we choose this way. It makes most cases easy, and some cases like this a bit harder. But unless it makes valid use cases impossible, it's imho a worthwhile tradeoff.
On Mon, May 9, 2016 at 9:40 AM, Krinkle krinklemail@gmail.com wrote:
The corruption I hypothesised is avoided because by design, page styles modules may not have dependencies. If a module 'foo' ignores that design requirement and changes regardless to require 'bar', then that change is in error and should be reverted.
https://gerrit.wikimedia.org/r/#/c/258662/ comes to mind, although you could play semantic games there with respect to what exactly depends on what since it seems pretty hard for a CSS file in isolation to actually depend on another CSS file versus the HTML output that uses the first CSS file also requiring the second for proper rendering.
wikitech-l@lists.wikimedia.org