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

🚩 Add strict flag #42

Closed
wants to merge 1 commit into from
Closed

Conversation

alecgibson
Copy link
Contributor

@alecgibson alecgibson commented Jul 12, 2021

Fixes #41

This change adds a strict flag to control whether we have the stricter
type 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 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
Copy link
Contributor Author

@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})

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.
Copy link
Contributor

@nornagon nornagon left a 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.

  1. 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.
  2. "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?

@alecgibson
Copy link
Contributor Author

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 ?

@josephg
Copy link
Member

josephg commented Jul 13, 2021

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
Copy link
Contributor Author

@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 http://sharejs.org/types/JSONv0?v=1.1.0)

@alecgibson
Copy link
Contributor Author

Closing this as per above discussion.

@alecgibson alecgibson closed this Jul 13, 2021
@alecgibson alecgibson deleted the strict-flag branch July 13, 2021 07:40
@alecgibson
Copy link
Contributor Author

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

  • in theory, if all of your ops are well formed, the new version of json0 is non-breaking
  • we're going to add a shim to handle historic ops
  • we want to let people smoothly transition to the new version of json0, and changing the URI type would force people to delete/recreate (or fudge their own create ops in the DB)

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.

Strict mode flag
3 participants