Am 28.01.2020 um 19:41 schrieb Pavel Rappo <[email protected]>:
>
> Hannes,
>
>> On 28 Jan 2020, at 17:15, Hannes Wallnöfer <[email protected]>
>> wrote:
>>
>> Hi Pavel,
>>
>> I fully concur with your reasoning for removing the jszip feature.
>
> Removing JSZip is incidental here. It's the whole "zipped index files" feature
> that is being removed.
>
Understood.
>> In script.js you left in the checks whether the *SearchIndex variables are
>> undefined:
>>
>> if (!moduleSearchIndex) {
>> ...
>> }
>>
>> These if-statements can be removed as the conditions will always be true
>> (the vars are declared a few lines above but not assigned any value, so they
>> will always be undefined). It’s ok to just invoke createElem(…) for all
>> scripts/index files.
>
> I will remove those conditional checks, thanks.
Actually my analysis was not correct since the variables are defined in
top-level script code while the script tag creation happens in function code,
so other things could happen between execution of the two. The conclusion is
still correct though since the *-search-index.js files is the only code that
assign to these variables.
>
>> I also wonder if there is any benefit left in creating the script tags
>> dynamically vs. just adding them to the pages statically. I’m fine with
>> keeping it that way for now, but maybe we could file another issue to check
>> if there’s any reason to keep this over static <script> tags.
>> Everything else looks good to me.
>> Hannes
>
> One benefit of creating those script elements dynamically seems to be the
> effectively
> asynchronous transfer of the .js index files. Those files should be
> transferred
> asynchronously, no question about that. We should make this explicit, though.
>
> I'm not a web developer, but from what I've read there is more than one way to
> load a script asynchronously. I wanted to muse over all the options properly
> in
> the context of 8236935: Improve UX of the search control, where this can be
> evaluated together with the different sorts of script loading events.
>
You’re right, async loading is probably the main benefit of loading scripts
this way. It seems HTML5 provides an `async` attribute to the script tag for
this purpose, and there’s also the older `defer` attribute for older browsers.
I think I’ll file a separate issue for this as the implementation detail of
async loading doesn’t really belong to a UX bug (unless we decide to switch to
synchronous loading, which I don’t think we should).
>
> I intend to let this RFR sit here for at least 2 weeks. This, hopefully, will
> make it accessible to more pairs of eyes. Maybe the original authors will
> chime in.
> The perceived severity of this change should not be underestimated.
>
Sounds like a good plan.
Hannes
> -Pavel
>
