ottypes / json0

Version 0 of the JSON OT type
449 stars 64 forks source link

🚩 Add `strict` flag #42

Closed alecgibson closed 3 years ago

alecgibson commented 3 years ago

Fixes https://github.com/ottypes/json0/issues/41

This change adds a strict flag to control whether we have the stricter type checking introduced in https://github.com/ottypes/json0/pull/40

Strict mode will be off by default (to maintain compatibility with old json0 versions).

In order to add this flag, we also add a new options object to the type. Strict mode is enabled on the type by setting the flag:

type.options.strict = true

Note that text0 will share the same options as json0 (ie enabling strict mode for json0 also enables it for text0).

In this change we also tidy up some unused utility functions from a previous commit.

alecgibson commented 3 years ago

@josephg / @nornagon can you please check you're happy with this API? I've nested the flag inside an options object, because it feels like it would be useful for future options, and potentially for clients trying to negotiate options with the server in ShareDB.

The one thing I'm unsure about in this PR is making text0 share the same options as json0. It's definitely simplest for the consumer to do it like this, but might be slightly confusing? (In theory a consumer could independently set text0 options by overriding the whole options object: text0.options = {strict: true})

alecgibson commented 3 years ago

Sure, I'm all for a v2 release. Was just trying to give the option of backwards compatibility if you wanted it. If we're breaking things, can we also look at merging https://github.com/ottypes/json0/pull/23 ?

josephg commented 3 years ago

I agree with @nornagon's concerns. And yeah, I'd be happy to merge #23 as well if thats the road we're taking.

I'm concerned about how the type should be named / registered. Is it "json0" still or "json0v2"? I think the former makes more sense, but I can imagine running into issues when loading old operations which don't conform to the new rules or have missing deleted data.

alecgibson commented 3 years ago

@josephg I've already run into these issues in https://github.com/share/sharedb/issues/494 — I'm hoping to discuss them later today at the ShareDB PR meeting.

I'm personally leaning towards renaming the type when we make a breaking change, so that you can register both versions separately (or we need to come up some way of adding semver to the URI and parsing that — in the past I think we've discussed naming types like http://sharejs.org/types/JSONv0?v=1.1.0)

alecgibson commented 3 years ago

Closing this as per above discussion.

alecgibson commented 3 years ago

@josephg / @nornagon I just discussed with @ericyhwang , and we think that it's best to just leave the URI the same, because: