Add String.prototype.matchAll polyfill
#1107
Conversation
There was a problem hiding this comment.
Thank you for this submission @mhassan1, this looks fantastic!
I've left a couple comments, one is around a missing dependency and the other is around the use of Object.assign in the tests
I'll kick off a run on CI now for this 👍
|
I addressed those two comments. I saw some failing tests in the first run. Do we need another run? |
mhassan1
changed the title
String.prototype.matchAll polyfillString.prototype.matchAll polyfill
|
The test failures look genuine to me. Have you seen the tests pass in those versions of internet explorer? |
|
@JakeChampion Can we please re-run checks? I have made some fixes. |
|
@mhassan1 It is now running the tests once more 👍 |
|
@JakeChampion I've just added a small fix for IE8. I believe all tests should pass now. Please re-run. |
|
@mhassan1 Rerunning now 👍 |
|
@JakeChampion It looks like all tests passed except for chrome, due to |
|
all tests pass @mhassan1 |
There was a problem hiding this comment.
This looks fantastic @mhassan1 - thank you so much for working on this!
I'm curious to know how you found authoring these polyfills within polyfill-library. I really like that you commented the spec steps within the polyfill files :-)
|
I enjoyed it! It was a great way to learn more about the spec and ES abstracts. I'd be happy to author others. |
Resolves #995