1101518 - [Build] Don't rewrite "async" scripts, and fix the skipping operation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Build, defect)

Description

[Julien Wajsberg [:julienw]]

Currently webapp-optimize.js has some code to be able to skip some scripts. However it's broken and we should do this for async-ed scripts by default.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: github PR

Hey Ricky, this is a simple change to prevent async scripts from being merged with other scripts.

Without this change async scripts are merged with non-async scripts, which sounds wrong.

I don't really know how to minify them so in this patch I didn't try to do it. Since I would like to uplift this to be able to use "async" in another patch on v2.1, I also wanted to keep the patch simple. Please tell me if you'd prefer that we simply use skipOptimize for this.

There is also a fix for skipOptimize; because the use of "splice" makes it skip the next script. I'm using "Array.from" but it doesn't work in unit tests, I don't know why.

Because I don't know how to fix unit tests I'm asking feedback only. I'd be glad if you can finish the patch by yourself.

Comment 2

[Ricky Chien [:rickychien] (inactive)]

I don't know the reason why we take Array.from since we already used var scripts = Array.prototype.slice.call to generate array in advance. I don't what's the bug you encountered here.

I agree with skipping "async" script but also skipping "defer" too. For me, checking script.hasAttribute('async') may not work if someone write <script async="false"...>.

Array.from only implement in Firefox not in node.js so that fails unit tests. (unit test is running on node.js)

And I don't really understand regarding minify. So ni George to give us feedback.

Comment 3

[Julien Wajsberg [:julienw]]

(In reply to Ricky Chien [:rickychien] from comment #2)
> I don't know the reason why we take Array.from since we already used var
> scripts = Array.prototype.slice.call to generate array in advance. I don't
> what's the bug you encountered here.

The issue is that you're splicing the same array you're iterating it. This does not work, this skips the next script too. You can try with a simple array in jsbin (too bad I lost my own experiment :) ).


> 
> I agree with skipping "async" script but also skipping "defer" too. For me,
> checking script.hasAttribute('async') may not work if someone write <script
> async="false"...>.

"defer" scripts already have a special behavior; they're all concatenated together and added back as a "defer" script. I think it's fine :)

> 
> Array.from only implement in Firefox not in node.js so that fails unit
> tests. (unit test is running on node.js)

Ah ok, then I can just use slice.

> 
> And I don't really understand regarding minify.

Currently minify is done in the optimization phase. Since this simple patch merely disable the optimization for async scripts (note: we don't have any in the codebase right now, because it didn't work) it's not minified either.
Because it's not used in the codebase yet I think it's fine to not minify until we see it can be improved.

> So ni George to give us
> feedback.

Comment 4

[(Inactive after June) George Duan [:gduan] [:喬智]]

I've left some comment in github. Thanks to point out scripts.splice bug ...

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8525313 [details] [review]
github PR

I think I've working unit tests now.

Note that the unit tests are really difficult to follow, it would be better to use sinon to control the results separately in each test...

From the github PR comments, here are my answers:
* we should not concatenate async scripts, because this would go against what async scripts are for
* long term, we should minify the async scripts, but I don't think this is not necessary to do this _now_ because there are no async scripts in this project yet and I'm not sure minifying async scripts would bring perf improvements. At least we should measure before doing it.

So I think we should move forward and land this now :)

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8525313 [details] [review]
github PR

Unit test is troblesome in build system now, so bug 1103844 will introduce sinon in the feature.

r+ and leaved a comment on Github.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8525313 [details] [review]
github PR

Hey Ricky,

I tried using `script.async` instead of `script.hasAttribute('async')` but it was true for all scripts... However I looked up the spec and found that because "async" is a boolean attribute, `async="false"` is really like `async`, and does not disable it (see [1] for all links to the spec). So after all I used `script.hasAttribute` again :)

However, trying `script.async` made me see that the previous fix to the use of `splice` was still broken and I fixed it for real now, using `filter`. I find it's also more elegant in the end :) I also added a unit test for this (I checked that it was failing with my previous fix).

Tell me what you think!

[1] https://github.com/mozilla-b2g/gaia/pull/26292#discussion_r21426780

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8525313 [details] [review]
github PR

Thanks your survey :)

I think filter is better than splice. r+ again.

Comment 9

[Julien Wajsberg [:julienw]]

master: d4e651d34448191a715ca4a7da829a03321245bc

thanks !