search

zspecza / common-tags

πŸ”– Useful template literal tags for dealing with strings in ES2015+
Other
1.99k stars 60 forks source link

Use indexOf instead of String.prototype.includes to support IE #112

Closed Shtian closed 6 years ago

Shtian commented 7 years ago

When run in IE the code throws an error since String.prototype.includes does not exist in IE, and it is not transpiled by babel either. Can we switch to use the cross browser compatible indexOf() instead?

codecov-io commented 7 years ago

Codecov Report

Merging #112 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #112   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          23     23           
  Lines         148    148           
=====================================
  Hits          148    148
Impacted Files Coverage Ξ”
...c/splitStringTransformer/splitStringTransformer.js 100% <100%> (ΓΈ) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update f651e9d...23588d4. Read the comment docs.

Shtian commented 7 years ago

Is this repository still being maintained? :)

ibrahima commented 7 years ago

Looks like the maintainer is recovering from an accident: https://github.com/nuxt/nuxt.js/issues/328 . I hope he's doing well!

Shtian commented 7 years ago

Well spotted @ibrahima, thank you. Best wishes for a good recovery @declandewet!

fatfisz commented 6 years ago

Hi! I joined this project as a maintainer today.

I'm not too sure about this change because while .indexOf(...) >= 0 works, .includes(...) is more modern, is a part of the official standard from 2 years ago, and is widely-supported.

In a scenario where you add a polyfill for String.prototype.includes in your project you will be able to easily remove it if you decide to drop support for IE 11.

If .indexOf(...) >= 0 is used here instead, coming up with the right time to remove it will be much harder.

So it's not only about changing the library code to support an old browser, but about who's responsible for making the code work in the older browser.

What's your take on this? Would you be willing to use a polyfill? Instead of this change I'd add a note about ES features that should be polyfilled.

Shtian commented 6 years ago

That sounds reasonable, @fatfisz. I agree with your statement, it's just the unfortunate few of us who have to cater for the remaining 3-4% on IE :) I've already solved this with a polyfill on my side, but a note about older browser support / polyfills in the readme would be great πŸ‘

Thanks for your comment, I'll close this for now.

fatfisz commented 6 years ago

I couldn't agree more with the "unfortunate" bit, at my work we also support IE 11 and there's a dedicated file with polyfills - I had to add another one to the list no further than yesterday...

I'll add a note about used features soon, thanks for your patience πŸ™‚