Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Lodash endsWith equivalent #2072

Merged
merged 1 commit into from
Jul 28, 2017
Merged

Use Lodash endsWith equivalent #2072

merged 1 commit into from
Jul 28, 2017

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 babel-preset-env@2.x (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.

4 participants