Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented Jul 28, 2017

Related: https://github.com/WordPress/gutenberg/pull/1674/files#r130091486
Related: #746

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/endsWith#Browser_compatibility

We do not use babel-polyfill or the like to polyfill ES2015+ base type prototype methods (e.g. Array#includes, String#endsWith), so we must be careful to use Lodash alternatives instead.

I'm not opposed to using a polyfill, though it introduces non-trivial bulk to the bundle size. This situation will improve in [email protected] (current alpha.16) with automatic feature detection.

https://github.com/babel/babel-preset-env/tree/2.0#usebuiltins-usage

Testing instructions:

Repeat testing instructions from #1674

@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Jul 28, 2017
@aduth aduth requested a review from ellatrix July 28, 2017 13:44
@nylen
Copy link
Member

nylen commented Jul 28, 2017

Can we make use of this and other problematic methods a lint error?

@aduth
Copy link
Member Author

aduth commented Jul 28, 2017

Can we make use of this and other problematic methods a lint error?

We can match the identifier, but it's hard to know the type, so it's possible to hit false positives with custom classes containing endsWith on their prototype, or the endsWith Lodash method itself.

Here was Calypso's take on the problem: Automattic/wp-calypso#6117

Honestly, if we expect to use polyfilling eventually anyways, I'm not inclined to spend much effort on it.

@mtias
Copy link
Member

mtias commented Jul 28, 2017

Looks good.

@aduth aduth merged commit b3c3bd6 into master Jul 28, 2017
@aduth aduth deleted the update/endswith branch July 28, 2017 15:06
@ellatrix
Copy link
Member

Thanks Andrew! I should've checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants