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 indexOf instead of String.prototype.includes to support IE #112

Closed
wants to merge 1 commit into from

Conversation

Shtian
Copy link

@Shtian Shtian commented Aug 28, 2017

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
Copy link

codecov-io commented Aug 28, 2017

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%> (ø) ⬆️

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 Shtian changed the title Use indexOf instead of Array.prototype.includes to support IE Use indexOf instead of String.prototype.includes to support IE Aug 29, 2017
@Shtian
Copy link
Author

Shtian commented Sep 1, 2017

Is this repository still being maintained? :)

@ibrahima
Copy link
Contributor

Looks like the maintainer is recovering from an accident: nuxt/nuxt#328 . I hope he's doing well!

@Shtian
Copy link
Author

Shtian commented Sep 12, 2017

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

@fatfisz
Copy link
Collaborator

fatfisz commented Nov 21, 2017

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
Copy link
Author

Shtian commented Nov 22, 2017

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.

@Shtian Shtian closed this Nov 22, 2017
@fatfisz
Copy link
Collaborator

fatfisz commented Nov 22, 2017

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 🙂

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

Successfully merging this pull request may close these issues.

None yet

4 participants