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

feat(877): matchall #1639

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ltruchot
Copy link

@ltruchot ltruchot commented Oct 17, 2020

Contains:

  • native curried matchAll
  • ponyfill curried matchAll
  • tests
  • typescript definition
  • export

Decisions/Tradeoffs:

  • native "matchAll" returns an iterable type: RegExpStringIterator. This is why most of the examples look like [..."testtest".matchAll("te")], spreading the iterable into a regular Array. Creating this kind of iterator in polyfill involves symbols, generators, and other ES6 elements that should be polyfilled too, in a long and not perfect chain. So I decided to directly return the Array as a result, making some rarely used optimization of matchAll impossible.
  • EcmaScript official documentation says that matchAll non global regex should throw an error (@see 21.1.3.12 in https://tc39.es/ecma262/#sec-string.prototype.matchall). Main browsers implements that behavior but node.js 12+ doesn't, so there is no test to "test this error" (except for the ponyfill that enforce the correct error).

@ltruchot ltruchot mentioned this pull request Oct 17, 2020
@ltruchot
Copy link
Author

@char0n about the codeclimate error: not sure if we want to avoid this glue-code in the ponyfill since there is no "helper" or "shared" folder here, but I can do it if you prefer!

@ltruchot
Copy link
Author

@char0n Some news about this feature ?

@char0n
Copy link
Owner

char0n commented Sep 29, 2021

@ltruchot sorry this fell through my net. Are you willing to continue working on this PR?

@ltruchot
Copy link
Author

I was satisfied with my PR, but I'm pretty sure if I read it now I'll found improvment to do. Can you let me few day to check that ?

@char0n
Copy link
Owner

char0n commented Sep 30, 2021

I was satisfied with my PR, but I'm pretty sure if I read it now I'll found improvment to do. Can you let me few day to check that ?

Sure, I'm sure it's a pristine work ;] I was more about if I do the code review (CR) and will have some suggestions or questions, if I should expect some answer, after this long time. But obviously I can ;] I'll get into CR.

Thank you for your work!

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

Successfully merging this pull request may close these issues.

None yet

2 participants