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

json5 for schema #690

Closed
toreyjs opened this issue Apr 12, 2017 · 13 comments
Closed

json5 for schema #690

toreyjs opened this issue Apr 12, 2017 · 13 comments
Assignees
Labels
Milestone

Comments

@toreyjs
Copy link

toreyjs commented Apr 12, 2017

I know it's not an official format, but I recommend adding json5 file support for use in schema. The json5 file type would allow for adding comments, trailing commas, and unquoted object keys (as well as other useful JS features currently missing from it). I know there's a few JSON5-php libraries on github that could be used, and would still backwards support use of json files (since json files are valid json5).

@alexweissman
Copy link
Member

alexweissman commented Apr 16, 2017

Hmm, interesting. I've actually been thinking that YAML might be a better alternative for our schema files, to be honest. YAML is easier to write (much simpler syntax), and it supports comments too :-)

@toreyjs
Copy link
Author

toreyjs commented Apr 17, 2017

YAML would be a good format to use as well. Mostly considered JSON5 since it would allow backward compatibility. Only other reason I can think of to push for JSON5 is so there would be one less extra file format used in the project (since JSON5 and JS essentially have same format), especially since no other formats in the project are currently indentation-based (which people either love or hate). Either would be an improvement in my book though! Mostly comes down to preference I imagine.

@alexweissman
Copy link
Member

Yeah, reading this thread, there doesn't seem to be a lot of love for JSON5. I see what you're saying though, since Composer, Bower, and the npm libraries we use, depend on JSON. I'm not sure how important it is that we maintain consistency with that, though.

@toreyjs
Copy link
Author

toreyjs commented Apr 19, 2017

That thread is comparing JSON vs JSON5, not JSON5 vs YAML, so various points are moot, like JSON5 adding needless complexity to JSON (since YAML already has that complexity plus more), and JSON5 not supporting datetime type (which YAML does not either to my knowledge, and is moot in this case). Thread also doesn't have much love for YAML, although most issues are moot in this case. Consistency isn't super important, but when comparing which to use it is something to consider (though not a deciding factor). Even though it doesn't matter on server end, JSON seems to be a bit of the standard for web stuff in general.

While my personal preference is JSON5, YAML does have various benefits (being a more accepted spec, having a native php parser, simpler syntax, etc) and may be the stronger option. Either would be an improvement I'd be happy with over JSON.

I think my preference for JSON5 is backwards compatibility; even though some YAML parsers read JSON, it could be a bit weird having two different formats in same project (if updating a project to UF4, but with all newer schemas using updated YAML syntax), while JSON5 is more or less same syntax with less strict rules.

@alexweissman
Copy link
Member

Since 4.1 will bring some breaking changes anyway, maybe it would be a good opportunity to switch to YAML? Or maybe we can support both formats?

@alexweissman alexweissman added this to the 4.1.0 milestone May 20, 2017
@toreyjs
Copy link
Author

toreyjs commented May 20, 2017

Should probably just stick with YAML to keep it simple (especially since JSON5 requires another library, even if small). Support for other formats can just be manually added on sites that need/desire it.

@alexweissman
Copy link
Member

Well, I just meant support for either YAML or JSON (classic).

@toreyjs
Copy link
Author

toreyjs commented May 20, 2017

Oh, I think yes. Isn't JSON actually a valid subset of YAML in YAML 1.2 actually (allowing a YAML parser to parse JSON)? Might support parsing both out of the box.

@alexweissman
Copy link
Member

This?

@toreyjs
Copy link
Author

toreyjs commented May 20, 2017

Yes. I also just tested it here and the parser accepted json. I'll admit I'm not sure if the php yaml functions support JSON or not though.

@alexweissman alexweissman self-assigned this May 30, 2017
@alexweissman
Copy link
Member

Ok, I've implemented basic support for reading YAML files in Fortress, on the develop branch of userfrosting/fortress. See the test cases: https://github.com/userfrosting/fortress/blob/develop/tests/RequestSchemaTest.php#L40-L62

I'm using Symfony's YAML parser, which does support parsing JSON, but unfortunately it can't handle the newlines in our JSON schema.

What I've done, then, is set up YamlSchema to try and parse as YAML first. If it fails, it will then fallback to try parsing as JSON.

Besides the request schema, where else can we think about using YAML as the default?

@alexweissman
Copy link
Member

I got the sense in chat, that generally people aren't too keen on my introducing another BC with a YamlSchema and JsonSchema. So, should we just stick with the single RequestSchema class that tries YAML first and then falls back to JSON?

@alexweissman
Copy link
Member

Alright, I have made my decision. By default, Fortress will first try to parse YAML and then fall back to JSON. In Fortress 4.1, we have decoupled the file loading from the schema itself, (YamlFileLoader vs RequestSchemaRepository), so developers will have the option to override this default behavior if they wish.

To discuss this further, please open an issue up directly on the Fortress issues page.

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

No branches or pull requests

2 participants