Built sims are failing in IE

[jbphet]

It was reported in phetsims/gravity-force-lab#192 that a built version of the GFL sim is failing to load. QA reports that they noticed the same thing with a recent dev version of Number Line: Integers. Based on the error output, it appears that something has changed with the build process recently that is causing ES6 features to end up in the build.

[jbphet]

Here is the Slack dialog where this was discussed and ultimately led me to create this issue:

John Blanco:office: 2:09 PM
Something seems to have changed about the build process that is causing builds to fail in IE.  Specifically, I'm seeing ES6 constructs in the build output.  Has anyone been messing around with this?  There a bit more detail in https://github.com/phetsims/gravity-force-lab/issues/192#issuecomment-551990136.
Sam Reid:house_with_garden: 2:10 PM
Does the non-debug version run OK?
Chris Malley 2:11 PM
Do recently-published (new) sims run on IE11?  Vector Additon, Curve Fitting, …
Kathryn Woessner:office: 2:13 PM
Yes.  They are all tested as part of the RC process
John Blanco:office: 2:13 PM
No, the non-debug version does not run, but the failure is somewhat different.
Chris Malley 2:13 PM
And yes, what’s on the website is working on IE11?
Sam Reid:house_with_garden: 2:14 PM
Can you show the error message or symptom for IE11 for non-debug version?
John Blanco:office: 2:14 PM
See https://github.com/phetsims/gravity-force-lab/issues/192.
Kathryn Woessner:office: 2:14 PM
Yes what is on the website works
:sweat_smile:
1

John Blanco:office: 2:16 PM
The problem in the non-debug versions of both GFL and NL:I is related to .from, which is not supported in IE according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from, so this also seems to point to something that has changed recently with the build process.

MDN Web DocsMDN Web Docs
Array.from()
The Array.from() method creates a new, shallow-copied Array instance from an array-like or iterable object.
Sam Reid:house_with_garden: 2:17 PM
The non-debug version may not be babel-ed, and in that case we wouldn’t expect it to work.
I’m seeing Array.from and Buffer.from.  Are you sure it is Array.from causing the problem?
But I agree Array.from is a likely candidate
John Blanco:office: 2:19 PM
Nope, I'm just going on the contents of the error output, and it doesn't explicitly specify Array.from, just .from , but it seems like a good bet.
Sam Reid:house_with_garden: 2:19 PM
Himalaya has been using Array.from since April 2017
John Blanco:office: 2:20 PM
I'm not sure what that means, but it sounds like I should create an issue and assign to...JO?
Chris Malley 2:22 PM
privatePredicate was added to QueryStringMachine by me on 3/7/19, so it’s unlikely to be that code specifically. (It’s been there awhile and was vetted.) (edited) 
Sam Reid:house_with_garden: 2:22 PM
can you determine the line number and column of the failure in the built version in IE?
oh wait it’s in the screenshot
:+1:
1

John Blanco:office: 2:23 PM
I'm creating an issue.
Chris Malley 2:24 PM
If I were to publish a production version of a sim right now (using master), would it be broken on IE? (edited) 
Sam Reid:house_with_garden: 2:24 PM
Line 1918 is the declaration of himalaya script.
John Blanco:office: 2:26 PM
CM - I believe the answer to your question is yes.
Chris Malley 2:26 PM
so probably a relatively recent change, and that should help with bisecting.
Sam Reid:house_with_garden: 2:27 PM
vector-addition-equations_en.html also has Array.from in 2 spots.
Did Gravity Force Lab recently start using RichText? (edited) 

RichText uses himalaya

[samreid]

Perhaps usage of RichText by accessibility triggered usage of himalaya, which is trying to call Array.from?

[jbphet]

Perhaps usage of RichText by accessibility triggered usage of himalaya, which is trying to call Array.from?

Good theory. The previous dev version of Number Line: Integers runs fine in IE, the most recent one doesn't, and I added a RichText element between the two versions.

[zepumph]

It looks like this error occurred because of this build line:

this.ariaLiveElements=[].concat(_toConsumableArray(this.ariaLiveContainer.children)),

and _toConsumableArray is calling Array.from.

In AriaHerald for phetsims/utterance-queue#1, I added this.ariaLiveElements = [ ...this.ariaLiveContainer.children ];. This feels like it may be an issue with Babel. I thought that it would be able to handle transpiling the spread operator. I will add an example to wilder and see how it goes.

[zepumph]

I didn't have to update Wilder, I just rebuilt it (because it uses the spread operator in WilderModel). That fails in the same way.

So it looks like the spread operator currently isn't supported by Babel by IE. I'm not sure how to handle this, but I'll mark for dev meeting.

[zepumph]

I think that the only reason it is breaking right now is because the spread operator is used very rarely. There are only 24 usages of /\[ \.\.\.\w/ in the project. I broke this recently because I added a spread operator basically to every SCENERY/Display instance (because or phetsims/utterance-queue#1).

[zepumph]

@samreid and I found that this was an issue with not only the spread operator, but with the spread operator on an HTMLCollection, as is being done in AriaHerald.

We think it isn't enough to just hand the case in himalaya, and it also isn't enough to change this usage in AriaHerald. We should do one of the following:

• Polyfill Array.from and potentially others as needed (MDN has https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#Polyfill)
• Polyfill using a babel package.

Tagging @jonathanolson because he has investigated using babel polyfill before. Also, if we end up creating a single preload that has all preloads, perhaps we want to include all polyfills, including the requestAnimationFrame one that is in scenery.

[KatieWoe]

This is blocking some testing in phetsims/qa#456

[jessegreenberg]

We think it isn't enough to just hand the case in himalaya, and it also isn't enough to change this usage in AriaHerald

Could you describe the reasoning? I have never had a need for the spread operator and would be OK saying it cannot be used if that resolves this issue.

EDIT: Was answered during meeting, above comments illustrate this is far more than just usage of teh spread operator.

[samreid]

@jonathanolson said it would be nice to look into the babel polyfills to accomplish this and potentially cover other cases we may need in the future. Babel has a way to select the parts you want for the polyfill.

@jonathanolson volunteered to investigate implementing this via babel polyfill. If that works well, great! If that runs into trouble, our backup plan will be to add our own preload for the MDN Array.from polyfill.

We have some sim publications coming up in mid-december. We will need a solution before then. We could implement the polyfill now as a (temporary?) workaround?

UPDATE: Since we want to create RCs in the next couple of weeks, we decided to add the preload polyfill now. Then we can investigate babel polyfills at our convenience.

[samreid]

@zepumph will create an issue to add the polyfill to PHET_CORE/polyfills or PHET_CORE/preloads

[zepumph]

The stop gap measure will be completed over in phetsims/chipper#817. Demoting priority on this issue.

[zepumph]

@jonathanolson, @samreid, and I ran into this with the es6 module upgrade over in phetsims/chipper#899. When upgrading to Babel 7, this error occurred again. This time babel was needing a Symbol polyfill to accomplish this. Instead we just used Array.from() inline instead of having a spread operator on an array-like item (HTMLCollection). Things are working well now.

@jonathanolson anything else here? I think we can close but wanted to give you one more chance to look things over.

[jonathanolson]

This looks good to me. Should we discuss ways to (a) better debug this in the future on IE, or (b) ways to catch this specific error if it re-occurs?

[zepumph]

To me this issue is largely stemming from a Promise polyfill that eats up our errors silently. If we were able to see the error sooner, perhaps this would be caught easier. I also think a CT testing device devoted to built IE tests could be really nice. That way we don't have to wait for RC to catch these. Were you thinking of something more specific?

Likely we should investigate core-js for our polyfill needs. Do you think that would have better error handling in its Promise implementation?

[jonathanolson]

We can just point IE towards CT and its results will show up! I think that might be on me, need to get remote desktop working.

No clue on what would be good for the Promise implementation.

[zepumph]

No clue on what would be good for the Promise implementation.

I think that core-js would be the first place I would look. I created phetsims/chipper#911 for investigation.

We can just point IE towards CT and its results will show up! I think that might be on me, need to get remote desktop working.

I pinged over in phetsims/aqua#80 (comment)

@jonathanolson, I'm ready to close. Over to you for a final inspection.

[jonathanolson]

Looks good to me.