-
Notifications
You must be signed in to change notification settings - Fork 247
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 serializing RegExp objects to strings #91
Comments
I've thought about this, but it isn't really a feature that anyone has been asking for or that seems to be lacking. Without that demand, this probably won't get implemented. (Also, who would want to write a parser for a regex expression?) |
Came here to suggest the same thing. Something like {
pattern: /.*_tests\.js/
} I think it would be enough to try to convert anything between two / into a RegExp, why would you need to parse the actual expression? |
Here's a few examples that would fail if we treated everything between two /\/
/(/
/)/
/[/
/*/
/+/
/?/ We would also need to parse trailing flags ( We could Also, ECMAScript doesn't support full PCRE. Specifically, it doesn't support recursion, look-behind, directives, conditionals, atomic groups, named capture, comments, or embedded code. So, JSON5 cannot represent PCRE without breaking ES5 compatibility. Ultimately, this means that platforms that support PCRE wouldn't be able to serialize PCRE as JSON5. The best way to serialize regular expressions is to convert them to strings. |
It still seems that there is some desire to implement regular expressions as first class values in JSON5. So, I'm going to elaborate on why I think it's a bad idea. ParsingI've already established that if you just treat everything between two First, you'd need to make sure that at least one character exists between the two // <- comment
/a/ <- RegExp Now, what happens if you want to match a /a\/b/ // matches 'a/b' Okay, so any occurrences of /a\\/ // matches 'a\\' It has an occurrence of Okay, we're done, right? Well, what about this regular expression? /a[b/c]d/ // matches 'abd', 'a/d' and 'acd' It's a valid regular expression with an unescaped /a\[bc]d/ // matches 'a[bc]d'
/a[b[c]d/ // matches 'abd', 'a[d' and 'acd'
/a[bc\]d/ // error: alternation group is never closed Okay, so in the first example the Does that cover our bases? I don't know, but for the sake of argument, let's just say it does. Let's simplify this into a list of steps:
Step 16 parses the flags according to the ES5 spec. This would allow implementation specific flags to be used (e.g. ES5 doesn't support the Besides the fact that this extremely minimal regular expression parser is still relatively complex, it doesn't actually parse the regular expression. There's no way to validate the regular expression in an implementation agnostic way without writing way more code. We would also need to define what happens if a platform doesn't support regular expressions or can't parse the regular expression given. It wouldn't know until it tried to parse it. So, all JSON5 documents that include regular expressions would have to be late-validated, and there would be no true JSON5 validator. Implementation Agnostic vs Implementation SpecificLet's say we did implement a full regular expression parser according to the ES5 spec. It would no longer be implementation agnostic, and we would expect all implementations of JSON5 to parse JSON5 regular expressions into an ES5 compatible format on its native platform. Further, by creating an implementation restricted parser you're castrating the more powerful regular expression engines supported by other platforms. No more named captures, no more comments, no more look-behind, no more recursion, no more Solution
RegExp.prototype.toJSON = function() { return this.toString() } That's all you need to do to write code like: var emailRegex = /^([a-z0-9_\.-]+)@([\da-z\.-]+)\.([a-z\.]{2,6})$/
console.log(JSON.stringify(emailRegex))
// output: "/^([a-z0-9_\\.-]+)@([\\da-z\\.-]+)\\.([a-z\\.]{2,6})$/" As an added bonus, that same line of code also applies to |
I've re-opened this issue to discuss whether JSON5 should serialize I'm not suggesting that we treat |
I think no... In README.md
If implement I have a other suggestion, create a package call |
If we embed RegExp or Date inside string, they are plain strings accepted by JSON, so no problem here. In fact, this "trick" is currently used on JSON sucessfully. The only thing is that JSON needs a special reviver function to interpret the strings that have embeded a RegExp or Date object, like https://github.com/piranna/ContextBroker/blob/master/lib/reviver.js
Not a bad idea, but this would mostly host reviver functions if we want to still being compatible with JSON or with JSON5 implementations without support for this extensions. Also, if we use embeded strings this revivers could be used with plain JSON too... |
I'm in the process of finalizing v1.0.0, so I've taken another look at Here's what the const result = JSON5.parse("{regex: '/a[b/c]d/i'}", {regExps: true})
// `result` is equivalent to { regex: /a[b/c]d/i } and { regex: new RegExp('a[b/c]d', 'i') }
Here's what the const result = JSON5.stringify({regex: /a[b/c]d/i}, {regExps: true})
// result === "{regex:'/a[b\/c]d/i'}"
It has not yet been decided whether this feature will make it into v1.0.0. Please let me know what you think. This is not an extension to the JSON5 document specification. It is only an extension to the API of this library. JSON5 implementations are not required to implement this API. You can try this out by using |
I like it :-) Being opt-in extensions would allow other implementations to not support it but just instead threat them as plain strings, that's a nice default :-) |
I like the intent, but I'm hesitant at the idea of different implementations supporting different opt-in features. If regexes and dates are in demand, maybe we should just Either way, I don't personally feel like this should be part of 1.0. IMHO, 1.0's primary goal should be to finalize & formalize what's already been there and what people have already been using. The rough edges you've been smoothing have been awesome! This feels like just a bit beyond the line from polish to new feature to me. All that said — don't consider my opinion a show-stopper at this point! You've been doing amazing work @jordanbtucker and I appreciate your leadership and ownership. Even if you move ahead with it, I'll take a page from Jeff Bezos's book and say "disagree but commit". =) Thank you again for your contributions! |
@aseemk Valid point about having differing implementations, especially since this is the reference implementation. This feature may be better left to another implementation. I'm still hesitant about allowing regex literals as I stated in my earlier rant under the heading Implementation Agnostic vs Implementation Specific.
|
I totally forgot about that rant. It's a good one! Then I guess I end up back with a conservative stance of "let's not take this on right now". =) |
Also great point about Date. Yuck. |
Funny thing about that rant. I actually used that algorithm to implement this experimental feature. |
@jordanbtucker how about just use i think |
@bluelovers I agree. That's what this experimental feature does. Lines 91 to 93 in a6c2b14
|
@jordanbtucker already have? but today i still get |
@bluelovers It has only been implemented in this experimental feature. It's not part of the main code. You can use this const replacer = (k, v) => v instanceof RegExp ? v.toString() : v
JSON5.stringify(/abc/g, replacer) |
also maybe for function too? not |
For functions, use this const replacer = (k, v) => v instanceof Function ? 'I tried to serialize a function with JSON5, but all I got was this stupid string.' : v
JSON5.stringify(replacer, replacer) // CUE THE BRAAAAM https://youtu.be/YoHD9XEInc0?t=1m1s Okay, enough kidding around. See #132, #158, and #106 (comment) |
Was the |
@gajus No, but you can use this reviver function to achieve the same result. https://github.com/json5/json5/blob/v1.0.0-regexps/src/parse.js#L51-L115 The Or you can use this self-contained regexp-reviver.js gist. |
This comment was marked as resolved.
This comment was marked as resolved.
@kussmaul Thanks for the suggestion, however the reviver function already works with arrays. The |
@jordanbtucker Oops, my bad, I should have looked more closely. Thank you for the prompt feedback! |
PCRE is now the main regex format, included into stdlibs of the most of languages. Why not to allow inserting literal regexes into JSON5 document?
The text was updated successfully, but these errors were encountered: