Add warning for ES6 instance functions in development environment

[umurkontaci]

I've been hit by mistakenly using a not polyfilled function (Array#find) and broke IE11 experience for a while.

There has been a number of painful experiences in the past, and I want no one to experience the same thing again.

We have two real options to the problem at hand:

• Include babel-polyfillFramework: Investigate using babel-polyfill #2360
• Break those functions in developer environments (failfast)

Both of these options boil down to the same thing: make the environments similar to each other. I am find with using either of them, as long as we have a solution. But need to pick a solution and move forward with it.

This PR approaches this by intentionally breaking the non-polyfilled functions in dev and staging environments for all browsers (Test on Chrome, Safari, Firefox). It iterates through the keys in the prototype object and deletes them.

The approach is a bit unconventional, but after putting my bias aside, I cannot think of an unwanted outcome out of this. Ideas welcome.

I've tried writing a eslint rule at first, but no static analysis tool is going to be reliable as it cannot know the type of the object every time.

I've put the file under lib/degrade-es6-fns but we can move it to a better place once we settle on the approach.

/cc: @rralian @gwwar @aduth @klimeryk

Test live: https://calypso.live/?branch=fix/es6-ie11

[blowery]

Perhaps instead of deleting the method from the prototype, we replace it with a method that throws an Error explaining why the method shouldn't be used? That would at least explain to a new dev why the method was elided.

We could also just polyfill the commonly breaking ones if they're missing. Thinking the methods on String and Array.

[umurkontaci]

@blowery I thought about it, but I wasn't sure whether having the keys in the prototype object would have a side effect.

On the other hand, if we can just polyfill the instance methods, it'd be easy to add a linter rule for static method calls.

Polyfilling the instance methods and eslinting static ones sounds like a good compromise to me.

[gwwar]

Let's stick with development. It's best if stage mirrors prod as closely as possible.

[gwwar]

I would prefer that we polyfill the common missing methods, so things just work and are available. It's especially jarring since we use Babel and most folks would assume all of this is in place anyway.

If we really want to use a lib like lib/degrade-es6-fns, I would recommend wrapping existing methods with a descriptive console.error instead of deleting them off the prototype.

[rralian]

I would prefer that we polyfill the common missing methods, so things just work and are available.

I agree, I think we should just add the polyfill. I understand there's some concern about filesize, but minified and gzipped it looks like it's 32k (grab the polyfill here to see). That would bring our build.js from about 170k to about 200k. It's not insignificant, but I'd pay that price to enable the features and avoid the problems we've run into. Would be great if we could include it dynamically based on feature-sniffing though. Not sure if that's a thing.

[blowery]

I'm more than happy to take the size hit. I think it's worth it.

[aduth]

I'll be the dissenting opinion - 32kb is the size of an entire jQuery, and looking at what it adds, I'd personally only use one or two with any frequency. Doesn't seem worth it, aside from resolving errors, but there are alternative solutions to this problem. Would love to see tooling to selectively include those polyfills we're actually using.

[blowery]

What's the actual size change in a prod build for us? I think we are already getting a good hunk of it with the runtime..

[umurkontaci]

I think the benefit is not the functions we'd use but to provide an ecosystem we can develop on confidently. We've been using lodash.find with no problems but every once in a while we forget that the function is not supported in IE and everything goes south.

Not very familiar with the details, but we should be able to include a polyfill conditionally just like we do for sections inside calypso. But if we include the polyfill conditionally, we cannot use babel-polyfill directly, since it adds more features than just filling a couple functions (it enables generators etc.), hence creating another discrepancy between browsers.

[aduth]

I think the benefit is not the functions we'd use but to provide an ecosystem we can develop on confidently.

I recognize this, but 32kb is quite large if that's what it ends up adding, so I'd rather we see to exhausting any alternative solutions before pulling this trigger.

[umurkontaci]

I recognize this, but 32kb is quite large if that's what it ends up adding, so I'd rather we see to exhausting any alternative solutions before pulling this trigger.

Gzipped difference is about 38kb. It is not very large compared to what we send other than code, yet it is still not quite small.

There is also https://github.com/paulmillr/es6-shim which results in 20kb difference gzipped. It lacks the ES5 shims but we probably don't need them anyway.

Do you think conditional import with 20kb extra would be OK?

[aduth]

Looks like there's some overlap between es6-shim and what we gain via babel-plugin-transform-runtime / core-js. Am I mistaken? Or, if so, would we be able to drop the latter? Or perhaps we can use the standalone options from es6-shim?

[umurkontaci]

I attempted to finish this but I don't really have a lot of bandwidth to explore all the options, I can either delegate this to one of you folks or I can add the shim.

[mcsf]

I'm usually in favor of using native implementations and the latest ES goodness, but in this case the polyfill seems to carry a hefty price tag.

Gzipped difference is about 38kb. It is not very large compared to what we send other than code, yet it is still not quite small.

That would bring our build.js from about 170k to about 200k. It's not insignificant, but

I don't think we should be using our current bundle size as a reference; we should instead strive to make our codebase and bundles lighter over time (with all the smart mechanisms like preloading that we're rolling out, etc.). With that in mind, making the build 18% larger is hard to justify when compared with the marginal benefits we get from the polyfill. We already use most of lodash, and it's hard to beat that library in terms of features. In other words, is there anything right now so compelling in the polyfill that isn't just the ability to rewrite xs.find( y ) as find( xs, y )?

[umurkontaci]

We already use most of lodash, and it's hard to beat that library in terms of features. In other words, is there anything right now so compelling in the polyfill that isn't just the ability to rewrite xs.find( y ) as find( xs, y )?

I think there is no benefit of using xs.find( y ) over find( xs, y ), but that wasn't the point for the proposition.

Historically, we've had multiple wreckages due to using Array#find and forgetting it is not implemented in IE. Those sometimes go unnoticed in reviews, and when they do, we have a JS version of a fatal error that is costing us a lot of users.

As we don't have measures in place that allows us to catch the errors early on, it is just too risky.

[klimeryk]

This might be a stupid suggestion, but what if instead of "downgrading" those missing ES6 functions, we "upgraded" them to the implementations from lodash? So Array.prototype.find would be implemented as something like return lodashFind( this, callback );. Since we are already using lodash throughout our code, the bundles' sizes would probably stay the same. Or am I missing something here?

[mcsf]

@umurkontaci: I probably wasn't clear at all, sorry.

I think there is no benefit of using xs.find( y ) over find( xs, y ), but that wasn't the point for the proposition.

Indeed, there isn't. The opinion I wanted to convey was that obtaining a native xs.find from the Babel polyfill is of little value when we can simply use lodash's implementation, the latter being actually more powerful (cf. lazywrapper, iteratee) than the former.

Historically, we've had multiple wreckages due to using Array#find and forgetting it is not implemented in IE. […] As we don't have measures in place that allows us to catch the errors early on, it is just too risky.

I understand, and I agree (in fact: the same happened to me yesterday). My opinion is that making the native calls fail is preferable to including the polyfill.

[umurkontaci]

Oh okay, that does make sense. Sorry about that confusion.

To be honest, I don't see a practical difference between either approaches, but this discussion has lasted forever and it is not the first PR on the same topic.

@blowery @rralian @gwwar @aduth @mcsf I believe we need to make a decision now. How strong do you feel on whatever approach you're supporting?

[mcsf]

How strong do you feel

… mildly? I mean, I do think adding babel-polyfill would be a step in the wrong direction due to its size, but I can probably be convinced otherwise, and I also won't oppose the decision if general consensus chooses the polyfill.

[blowery]

This might be a stupid suggestion, but what if instead of "downgrading" those missing ES6 functions, we "upgraded" them to the implementations from lodash?

The semantics are slightly different. lodash assumes a continuous array with no holes, while the spec compliant versions support sparse arrays. This is one of the reasons the lodash version can be faster than the "native" version for solid arrays: there's no need to do the in check on every index you're looping over.

[blowery]

At this point, I'm voting for making the "missing" methods bomb. I'm leaning towards replacing them with versions that throw rather than deleting them, because then it's obvious why the call is "failing".

[umurkontaci]

I'm leaning towards replacing them with versions that throw rather than deleting them, because then it's obvious why the call is "failing".

I'm a bit worried of potential side effects this might bring. There might be a library in the wild which tries to use Array#find with a fallback to a local implementation. If we delete Array#find that library will know that the method doesn't exist and use a fallback method, however if we modify to throw, it will try to use our method and fail.

[blowery]

mmmmm good point.

[umurkontaci]

I've updated the code to log an error to the console upon calling the functions.

[gwwar]

We should return the value of the original function. For example, [ 'foo' ].keys() should return an array iterator. When wrapped, this currently returns undefined.

[umurkontaci]

Oh, lol. My bad.

[gwwar]

This should be good to 🚢 once we fix the return value of the wrapped methods.

[umurkontaci]

Just to make sure we are not breaking any stuff, I've added some tests and it is breaking a great amount of tests. Can't pinpoint to origin, will take a look next week at but could use another eye as well.

[gwwar]

Quickly looked at one of the traces, and it seems like one of our vendor libs is callingcodePointAt with unexpected results.

[umurkontaci]

I think it was the tests that were borked, I was stubbing the methods instead spying. The tests are OK now.

[umurkontaci]

@gwwar Did you have a chance to review this?

[gwwar]

Thanks for working on this. Looks like everything is happy on the branch @umurkontaci let's

[gwwar]

@umurkontaci let's rebase and