Skip to content

Conversation

@delvedor
Copy link
Member

@delvedor delvedor commented Aug 7, 2016

JSON by default does not support RegExp objects or strings, I propose to add RegExp type to the schema types.

reg: {
  type: 'RegExp'
}

In this way fast-json-stringify will transform regex object in escaped strings and not in '{}' as JSON.stringify does.

Example:

// Before
/"([^"]|\\")*"/
// After
'"\\"([^\\"]|\\\\\\\\\\")*\\""'

// Then
/"([^"]|\\")*"/.source === new RegExp(JSON.parse('"\\"([^\\"]|\\\\\\\\\\")*\\""')).source // true

What do you think?

@mcollina
Copy link
Member

mcollina commented Aug 8, 2016

type: 'RegExp' is not part ot jsonschema. How about adding this behavior to type: 'string'?

@delvedor
Copy link
Member Author

delvedor commented Aug 8, 2016

Yeah, you are right.
But there's a little drawback:
If the variable is an instance of RegExp it's easy transform it in an escaped string, but if it's a string that represents a regex, how could you know it?

For example:

// $asString function
str = /"([^"]|\\")*"/
if (str instanceof RegExp) {
  $asRegExp(str)
}
// $asString code

If the user passes '"([^"]|\\\\")*"', that is the string that represents the above regex, the \will not be escaped, so this string becomes '"\\"([^\\"]|\\\\\\")*\\""'.
If so new RegExp(JSON.parse('"\\"([^\\"]|\\\\\\")*\\""')).source is not the same of the original regex.

The only solution I see here, is to escape \\ in $asString* as well.

@mcollina
Copy link
Member

mcollina commented Aug 8, 2016

This is json-schema, it's a spec :/. So, +1 for making it a part of $asString. Open a separate issue for escaping, because I fear that might have bugs in general (for / as an example).

@mcollina
Copy link
Member

mcollina commented Aug 8, 2016

or \ maybe :).

@delvedor
Copy link
Member Author

delvedor commented Aug 8, 2016

Sorry, made a little mess with commit messages :P

Is better now?
If the user pass a regex object, the regex will be parsed into a string.

@mcollina
Copy link
Member

mcollina commented Aug 8, 2016

That's ok. I'll merge when I get back. You can do a git rebase to squash all commits into one.. :)

@delvedor
Copy link
Member Author

delvedor commented Aug 9, 2016

Done, thanks!

@mcollina
Copy link
Member

mcollina commented Aug 9, 2016

Can you please add some notes in the README about this?

@delvedor
Copy link
Member Author

delvedor commented Aug 9, 2016

Sure, which one you prefer?

Date instances are serialized with toISOString().
RegExpinstances are serialized as string.

or

Instance Serialized as
Date string via toISOString()
RegExp string

In my opinion the second is better, is more clean.

@mcollina
Copy link
Member

mcollina commented Aug 9, 2016

Agreed, do the 2nd.
Il giorno mar 9 ago 2016 alle 14:54 Tomas Della Vedova <
[email protected]> ha scritto:

Sure, which one you prefer?

Date instances are serialized with toISOString().
RegExpinstances are serialized as string.

or
Instance Serialized as
Date string via toISOString()
RegExp string

In my opinion the second is better, is more clean.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#1 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADL48zdgeEAaBa5tQ4uoH6RuCOtSoWgks5qeIatgaJpZM4Jemf6
.

@mcollina mcollina merged commit 791f427 into fastify:master Aug 14, 2016
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.

2 participants