automerge / automerge-repo

MIT License
419 stars 43 forks source link

Re-export commonly used Automerge core types through Automerge Repo #340

Closed pvh closed 2 months ago

pvh commented 2 months ago

In order to avoid problems created by importing mismatched versions of automerge's core CRDT library (and just the annoyance of having an extra import statement), in this PR I'm re-exporting "commonly used" Automerge types and functions.

Arguably some of these should just be better baked into DocHandle or something but I didn't want to let perfect be the enemy of good here and this is definitely an improvement.

Rarely used or more "internal" types and functions are also reexported via the AutomergeCore export which simply forwards on everything in next for now.

Questions for reviewers: did I miss any functions or types you commonly use? (We can always add more later.) Is there a better way to re-export this stuff? Do you hate AutomergeCore as the re-exported name? (I am leaning towards rebranding the existing automerge-js library to "core" as repo matures and making it the main "Automerge" export in a future release to hopefully reduce some confusion for new users.)

HerbCaudill commented 2 months ago

I don't love "AutomergeCore" - I think it's confusing.

If it's important to curate which parts of the library you reexport, could we just export the individual methods without namespacing them?

Alternatively, is there a reason not to just reexport Automerge as-is and with the same name?

pvh commented 2 months ago

I don't love "AutomergeCore" - I think it's confusing.

Me either! Discussion of that below.

If it's important to curate which parts of the library you reexport, could we just export the individual methods without namespacing them?

I am indeed doing that, unless I've made a mistake? The types & methods exported will be in the Automerge-Repo namespace.

Alternatively, is there a reason not to just reexport Automerge as-is and with the same name?

The AutomergeCore is meant to be slightly awkward and should be unnecessary for almost everyone unless you're doing some kind of weird hackery like decoding binary network messages to inspect or bypassing the automerge-repo APIs for some reason.

I wanted to avoid using the name Automerge for the export because we intend to rename automerge-repo to automerge in an upcoming release (or conceptually, merge repo into Automerge core if you prefer to think of it that way.) In basically all cases, what we call automerge-repo now is what people should be using for any new project that doesn't want to specifically reimplement its features.

As to why re-export it at all, I've received reports a few times that folks importing both automerge-repo and automerge can wind up with mismatched versions and experience confusing bugs as a result. This guarantees you get a matching version.

So the options I can see are: 1) don't re-export the core library 2) splat Automerge into Repo's namespace (when I saw how much this would clutter the namespace with duplicate functionality i shied away from this) 3) export it as Automerge (i feel this is even more confusing for new users) 4) export it under some other name (which as we've discussed is kind of gross)

Of all of these not-great options I figured (4) was the most reasonable: we don't want to advertise or encourage the use of that re-export but it's there if you need it. We could name it something else like AutomergeInternals, or Automerge_DONT_USE_THIS or something.

But before we bikeshed any further on names would you agree that (4) is the least objectionable option? I have relatively weakly opinions here. I also hate exporting a bunch of bare functions -- it's hard to get away from with the string manipulation stuff because JS strings aren't objects -- but maybe I've missed something.

What do you think?

HerbCaudill commented 2 months ago

I am indeed doing that, unless I've made a mistake? The types & methods exported will be in the Automerge-Repo namespace.

aha sorry I was responding on my phone earlier and didn't catch that.

At any rate I do think that option 4 is more objectionable than any of options 1-3.

  1. Could the problem of mismatched versions be solved by making Automerge a peer dependency instead of a direct dependency? Or some other way - surely we're not the first library in the JS ecosystem to need to do this.
  2. Pretty sure this would be my preference. The mess of mashing the two namespaces together is unfortunate, but if they're going to be merged eventually anyway then that's our reality and we need to start dealing with it.
  3. I would not be confused at all by an Automerge export, because there's only one thing that it could possibly be. Whereas any other name makes me think that it is something other than Automerge.
pvh commented 2 months ago

As for peer dependencies and such... I don't think anyone should ever have to import both @automerge/automerge and @automerge/automerge-repo. If you need something to work with Automerge Repo it should be exported from Automerge Repo! (Also, just pragmatically figuring out this dependency stuff is an endless tarpit of changing bundlers and standards and the people who have reported problems have definitely been experienced and knowledgeable JS devs so I don't think this is a trivial fix.)

I'm not comfortable with (2) because I don't want to export functions that you shouldn't use as part of Repo's namespace. For example, having handle.change() and also exporting the underlying doc = change(doc, callback) function in Repo's namespace... seems like a recipe for disaster.

Broadly speaking, I'm trying to prioritize legibility of the library for new users. It's not about me or you...

I guess the closest thing we have to common ground is (1). I think exporting an object called Automerge full of things you shouldn't touch is a DX landmine (as would be dumping everything into the root namespace) and you think renaming the export is a non-starter.

I strongly believe it would be a mistake to defer making improvements on the status quo until / unless we can schedule a redesign of the relationship between repo/core so... I think that just leaves (1).

WDYT?

pvh commented 2 months ago

Okay -- I'm going with (1) for now and only reexporting the specific types & functions above. Thanks for your thoughts all, the patch is better for 'em.