-
Notifications
You must be signed in to change notification settings - Fork 63
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
🚩 Add strict
flag
#42
Conversation
@josephg / @nornagon can you please check you're happy with this API? I've nested the flag inside an The one thing I'm unsure about in this PR is making |
This change adds a `strict` flag to control whether we have the stricter type checking introduced in ottypes#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: ```js 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... I don't love this, for a couple of reasons.
- It's not obvious with this API how to mix strict-mode and not-strict-mode in the same process. Granted, that's possibly an odd requirement, but e.g. it would preclude migrating data one "area" at a time.
- "strict" mode ought to be the default.
Might it make sense, rather than having the notion of a "strict" mode, to simply push the stricter requirements with a semver/major bump?
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 #23 ? |
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. |
@josephg I've already run into these issues in share/sharedb#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 |
Closing this as per above discussion. |
@josephg / @nornagon I just discussed with @ericyhwang , and we think that it's best to just leave the URI the same, because:
|
Fixes #41
This change adds a
strict
flag to control whether we have the strictertype checking introduced in #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 thetype
. Strict mode is enabled on the type by setting the flag:Note that
text0
will share the same options asjson0
(ie enablingstrict mode for
json0
also enables it fortext0
).In this change we also tidy up some unused utility functions from a
previous commit.