Skip to content

Switch from relying directly on Replicache to an interface.#35

Merged
aboodman merged 1 commit intomainfrom
subscribable
Sep 15, 2022
Merged

Switch from relying directly on Replicache to an interface.#35
aboodman merged 1 commit intomainfrom
subscribable

Conversation

@aboodman
Copy link
Copy Markdown
Contributor

This makes it easier to use with Reflect.

This makes it easier to use with Reflect.
@aboodman aboodman requested a review from arv September 15, 2022 11:54
@aboodman aboodman merged commit 3ee3fbe into main Sep 15, 2022
@aboodman aboodman deleted the subscribable branch September 15, 2022 11:56
Comment thread package.json
"react": ">=16.0 <18.0",
"react-dom": ">=16.0 <18.0",
"replicache": ">= 10.0.0",
"replicache": "10.0.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect this to be ^10.0.0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well here's the way I think of it -- we're not actually relying on a particular version . This code is basically do the same thing as the Go pattern of having consumers declare the interfaces for their dependencies, except that in this case the interface is so complex (ReadTransaction) that I'm borrowing the definition in Replicache 10. I could actually pick any version of Replicache that has an interface that is compatible with what useSubscribe is trying to do. I sort of arbitrarily picked 10.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

I was just commenting on locking the version like this. Generally you'd want to use semver.

Copy link
Copy Markdown
Contributor

@arv arv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

aboodman added a commit that referenced this pull request Nov 17, 2022
)"

This reverts commit 3ee3fbe.

I'm putting this back the way it was. In retrospect, this way of doing it
seems more consistent with the JS ecosystem. The package should specify
the versions it has been tested and known to work with explicitly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants