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

Consider returning a string representing the pattern instead of a RegExp object #3

Closed
mathiasbynens opened this issue Dec 22, 2016 · 5 comments

Comments

@mathiasbynens
Copy link
Contributor

mathiasbynens commented Dec 22, 2016

I’m assuming this project’s main usage scenario is in build scripts (rather than using it dynamically at run-time), just like Regenerate. Is this correct?

If so, how about returning a string representing the pattern instead of a RegExp object? This seems more useful:

  • Such a string can easily be turned into a RegExp object by calling new RegExp(string). (But this is only useful at run-time, which I think is not the main use case of this project.)

  • Strings compose more easily. Consider a build script that does something like…

    const output = `/(?:${ regexgen(words) })+(${ regexgen(someOtherWords) })/g`;

Instead, currently you’d have to call RegExp#toString (which is surprisingly buggy) on each of regexgen’s return values and remove the first and last / as well as the flags, if any, or use .source.

You could then also remove the logic for supporting flags, as the user would just deal with that themselves.

mathiasbynens added a commit to mathiasbynens/regexgen that referenced this issue Dec 22, 2016
@devongovett
Copy link
Owner

Fixed by #6. Released in v1.2.0.

@mathiasbynens
Copy link
Contributor Author

#6 didn’t fix this — it just made it easier to fix it. This issue is a change request about the regexgen return value. TL;DR IMHO, it makes more sense to have regexgen(foo) return a string instead of a regular expression object. WDYT?

@devongovett
Copy link
Owner

Another option would be to just return the Trie from regexgen. Then the user could call toString or toRegExp themselves.

I do like the convenience of just returning a RegExp though. You could do regexgen(words).source to get a string without the slashes when combining. Is your main concern with bugs in JS implementations? The one you linked seems to have been fixed in v8. Are there others you know of?

@mathiasbynens
Copy link
Contributor Author

Another option would be to just return the Trie from regexgen. Then the user could call toString or toRegExp themselves.

I like this option a lot. As a bonus, doing so would match the Regenerate API. :)

Is your main concern with bugs in JS implementations […]

I don’t know of any others off the top of my head, but if the risk of JS engine bugs can be avoided, then that’s a plus in my book.

That said, my main concern is that the API — in this case, the return value — should match the common use case, not the uncommon one. But perhaps I’m wrong in thinking that getting the pattern as a string is the most common/sensible use case.

I do like the convenience of just returning a RegExp though.

Could you point me to some examples of where you’re using this pattern (see what I did there)? Are you using regexgen at runtime?

@devongovett
Copy link
Owner

devongovett commented Dec 22, 2016

Are you using regexgen at runtime?

No, but my build scripts don't need to combine regexes. So I just do regexgen(words, 'g') instead of something like '/' + regexgen(words) + '/g'.

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

No branches or pull requests

2 participants