Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-10 Thread Krinkle 
Next steps now written up at
https://phabricator.wikimedia.org/T92459#2281878

On Mon, May 9, 2016 at 7:45 PM, Krinkle  wrote:

>
>
> On Mon, May 9, 2016 at 7:17 PM, Brion Vibber 
> 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
>
>
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-09 Thread Krinkle 
On Mon, May 9, 2016 at 7:17 PM, Brion Vibber  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
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-09 Thread Brion Vibber 
On Monday, May 9, 2016, Krinkle  wrote:

> On Wed, May 4, 2016 at 1:43 PM, Brion Vibber  > 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:  (loads site styles), mw.loader.load('site')
> - (loads site scripts and styles)
>
> Step 1:
> * Create 'site.styles'
> * Output A behaviour unchanged
> * New Output B: , 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: , 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  > 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 
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-09 Thread Jon Robson 
Okay great! :)
I got lost in the details. Lets make it so!

On Mon, May 9, 2016 at 6:50 AM, Krinkle  wrote:
> On Sun, May 8, 2016 at 5:47 PM, Jon Robson  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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-09 Thread Brad Jorsch (Anomie) 
On Mon, May 9, 2016 at 9:40 AM, Krinkle  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.

-- 
Brad Jorsch (Anomie)
Senior Software Engineer
Wikimedia Foundation
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-09 Thread Krinkle 
On Sun, May 8, 2016 at 5:47 PM, Jon Robson  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

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-09 Thread Krinkle 
On Wed, May 4, 2016 at 1:43 PM, Brion Vibber  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:  (loads site styles), mw.loader.load('site')
- (loads site scripts and styles)

Step 1:
* Create 'site.styles'
* Output A behaviour unchanged
* New Output B: , 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: , site.styles=ready,
mw.loader.load('site') - (loads site scripts)


On Wed, May 4, 2016 at 1:43 PM, Brion Vibber  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
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-09 Thread Krinkle 
On Wed, May 4, 2016 at 4:23 PM, Brad Jorsch (Anomie) 
wrote:

> On Wed, May 4, 2016 at 6:34 AM, Krinkle  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) 
 wrote:

> On Wed, May 4, 2016 at 6:34 AM, Krinkle  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) 
 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.
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-08 Thread Bartosz Dziewoński 

On 2016-05-04 12:34, Krinkle wrote:

## Proposed solution

...

Thoughts?


Yes. Please. Let's do that!

--
Bartosz Dziewoński

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-08 Thread Jon Robson 
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  wrote:
> On Wed, May 4, 2016 at 8:23 AM, Brad Jorsch (Anomie) 
> wrote:
>
>> On Wed, May 4, 2016 at 6:34 AM, Krinkle  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.
>>
>
> H... 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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-04 Thread Brion Vibber 
On Wed, May 4, 2016 at 8:23 AM, Brad Jorsch (Anomie) 
wrote:

> On Wed, May 4, 2016 at 6:34 AM, Krinkle  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.
>

H... 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

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-04 Thread Brad Jorsch (Anomie) 
On Wed, May 4, 2016 at 6:34 AM, Krinkle  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  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.

-- 
Brad Jorsch (Anomie)
Senior Software Engineer
Wikimedia Foundation
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] ResourceLoader addModuleStyles() issues

2016-05-04 Thread Brion Vibber 
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  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  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