feat: rows and rowsSync API on Shape

[msfstef]

Fixes #1888

I've added both a rows and rowsSync API to match what we already have for value and valueSync.

The react hooks already expose a data field that is the rows (it was doing the conversion there), so I don't think we need to change that API.

That being said, currently the subscribe API also returns the Map form of the data - if we want that to return rows and phase out the Map format entirely, it will be a significant, breaking change.

[samwillis]

Looks good.

One comment would be that we may want to change more of the tests to use .rows and .rowsSync as they are the user facing APIs.

[msfstef]

we may want to change more of the tests to use .rows and .rowsSync

I agree but that was my point about the subscribe API which currently returns the Map version of the data. If we want to push users to the rows version then we should also change the subscribe API otherwise it might be a bit confusing.

I'm happy to open a new PR with the full breaking change refactor.

[samwillis]

Ah yes. I would argue we should make this braking change now. But maybe this so that its more extendable in future:

export type ShapeData<T extends Row<unknown> = Row> = Map<string, T>
export type ShapeChangedCallback<T extends Row<unknown> = Row> = (data: { // <-- new wrapper object
  value: ShapeData<T> // <-- can still get the "value"
  rows: T[]  //  <-- now have the rows
}) => void

so that it can used with:

shape.subscribe(({ rows }) => {
  // Do stuff...
});

We can then add additional metadata as further props to the callback in future.

@KyleAMathews what do you think?

[thruflo]

Sidebar: rowsSync and valueSync — is this synchronous access to the rows/value? Ie wrap up await shape.value?

Because if so “sync” is a bit overloaded/ambiguous in our context.

[msfstef]

Because if so “sync” is a bit overloaded/ambiguous in our context.

I do think that the combination of sync being the suffix, that it is a getter, and of the common JS convention for synchronous API equivalents is enough to differentiate it from our general data sync concept.

That being said, the rowsSync and valueSync are not synchronous wrappers of the promise version - they access the current materialized shape regardless of its "up to date" status. Therefore they are "unsafe" in the sense that you might see transactionally incomplete data. In that sense I would agree that it might make sense to use a different term, like rowsUnsafe or something.

[thruflo]

Yeah, maybe currentRows / currentValue?

[msfstef]

Ok so based on this feedback I'm applying @samwillis suggestion (that is still a breaking change but keeps both the Map and rows version), and taking this opportunity for a breaking change to rename valueSync (and the new rows one) to currentValue and currentRows.

[KyleAMathews]

I like your api suggestion @samwillis 👍

And yeah, we're making a number of breaking changes now — so toss this on the pile. This is a great time to fine-tune the API to a huge extent as we have enough breadth and depth of experience to make better judgements on stuff like this.

[netlify]

✅ Deploy Preview for electric-next ready!

Name	Link
🔨 Latest commit	e267886
🔍 Latest deploy log	https://app.netlify.com/sites/electric-next/deploys/6720ea20e62fa80008e084ee
😎 Deploy Preview	https://deploy-preview-1897--electric-next.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[msfstef]

should be ready to merge now - updated the changeset to reflect the breaking change

[KyleAMathews]

neat!