tomasklaen / statin-preact

Preact bindings for statin.
3 stars 0 forks source link

Suspense / support for thrown promise + error boundary? #1

Closed danielweck closed 2 years ago

danielweck commented 2 years ago

Hello, I will be experimenting with your library, thank you for sharing your work :)

I've been playing with Valtio by @dai-shi which is a proxy-based solution (unlike Statin and Preact's own internal "reactive state" experiment).

Valtio supports (P)React Suspense by throwing Promises when accessing async state properties.

Have you considered the ramifications of Suspense/ErrorBoundary in Statin, which as I understand it is a synchronous reactive model? Any thoughts about potential caveats?

(I will be taking a deep dive some time soon, but I just wanted to ping you first)

Thank you! :)

Valtio readme:

https://github.com/pmndrs/valtio/blob/main/readme.md#suspend-your-components

PS: feel free to move this to a GitHub "discussion", as this is not really an issue or feature request.

tomasklaen commented 2 years ago

Yes, statin is a completely synchronous reactive model, similar to mobx, so there are no async state properties, therefore statin is not concerned with any async stuff happening around it, including suspense.

If there is anything that needs to load asynchronously, I create a union signal, and display a loader while it's still null, something like this:

const data = signal<Data | null>(null);

const Component = observer(function Component({data: dataSignal}: {data: Signal<Data | null>}) {
  const data = dataSignal();
  return data == null ? <div>...loading...</div> : <RenderData data={data}>
});

render(<Component data={data} />, root);

So I'd say suspense is outside of the scope of statin, but if you share some specific problem to solve, I might be able to help.

danielweck commented 2 years ago

Thank you for your prompt reply :)

Yes, an "old school" ternary works perfectly fine in most cases.

As the reactive data is external to the (P)React component tree, it should indeed be technically possible to manage asynchronous state updates (e.g. HTTP fetch) "outside" synchronous data getters ... I'll have to play a bit more with Statin to figure all this out 👍

I imagine that I would be using Statin's action() (or createAction() shorthand) to trigger a reactive state change when a promise is resolved / rejected (e.g. HTTP fetch), so architecturally it really comes down to how the async behaviour is initiated (i.e. at which point in the data flow and/or component lifecycle the promise is created, and where it is cached / managed). Thus the useful mental model of "suspense", and the implications in terms of render-fetch patterns: https://reactjs.org/docs/concurrent-mode-suspense.html#traditional-approaches-vs-suspense

At this moment in time I am primarily interested in basic support for Suspense / Error Boundary (to propagate "pending" state upwards in the component tree, to handle cascaded / nested resolutions, etc.). The "concurrent rendering" / state "transitions" story in React 18 is still in flux, and I am not too sure about the Preact 11 refactoring yet.

That being said, the switch from "useMutableSource" to "useSyncExternalStore" to reconciliate externally-managed data with (P)React component tree state is relevant to Statin, I think. More on the subject:

PS: once again, feel free to move this issue to a GitHub "discussion", it would probably be more appropriate there 😃

danielweck commented 2 years ago

Hello, I implemented support for Suspense / Error Boundary / thrown Promises in my Statin fork (currently integrated deep inside an open-source side project of mine, note that I have no intention of publishing anything to NPM).

In a nutshell, I simply added this code guard / bailout:

https://github.com/tomasklaen/statin-preact/blob/ea430a280f1577a7ae80aec5a030765ee3542e78/src/index.tsx#L172-L176

if (exception) {
    // "thenable" / Promise pass-through (handled by Suspense / Error Boundary higher up in the component tree)
    if (typeof (exception as Promise<void>)?.then === 'function') {
        throw exception;
    }
// ...

I also created tests to demonstrate that this works, although I still need to cover nested edge cases and various other stale/cleanup situations. I could attach test snippets in this GitHub comment, but that would be pointless because I significantly refactored both the original Statin codebase (same internal logic, but very different file organisation and slightly modified surface API + typings), as well as the original testsuite which I ported to vitest (based on an ESM / non-CJS local clone of @testing-library/preact in order to work around the Preact Hooks ESM/CJS mistmatch bugs caused by inconsistent handling of "import maps" + main/require/module/import/browser package.json fields).

In Statin, should you wish to support Suspense, I would suggest using Preact's own createSuspender() in order to put together a minimal test. Personally I use my own "cached async query" suspension micro-lib, but of course it is easy-enough to write a component that updates its internal state from "loading" to "ready" depending on a resolved Promise. https://github.com/preactjs/preact/blob/36be614da85de5c938ee02b8baaecf5f18513a8e/compat/test/browser/suspense.test.js#L15 Note that the original Preact render function is necessary to handle the Suspense / error boundary (the Suspense tests in Ava or Vitest won't run properly with the render() function from @testing-library/preact).

Side note: my project mixes Preact WMR / preact-iso (Lazy, ErrorBoundary, Router) and preact/compat's Suspense/Lazy ... which is typically problematic due to competing options._catchError Preact "hooks". However in my forked Statin tests, all that matters is that a thrown Promise is treated differently from a regular Error / exception, thereby allowing the error to propagate up the component tree instead of being handled locally in Statin's stack (typically, the inversion of control is implemented by a root error boundary that suspends the throwing component to allow a fallback render, but for example preact-iso uses a different pattern).

I am afraid I have run out of time to create a Pull Request here, otherwise I would have made this small contribution myself. Thank you so much for sharing Statin, I have learned a lot from your code ... standing on the shoulders of giants, MobX, Solid, etc. :)

tomasklaen commented 2 years ago

Oh I actually never touched or worked with suspense before, I see how it works now.

If all that is needed to add support for it is to just re-throw if the error payload is a promise, than I have no issue adding it in.

But I have a little issue comprehending the rest of your message about vitest, wmr, iso, compat, etc... :) I guess these are suggestions how to test this reliably? In which case I guess I'd be fine with not having a test for this one thing. It seems like a lot to mess with :)

I significantly refactored the original Statin codebase

Could you share why? Is there something with statin that was such a deal breaker that you had to rewrite it yourself? :)

danielweck commented 2 years ago

I significantly refactored the original Statin codebase

Could you share why? Is there something with statin that was such a deal breaker that you had to rewrite it yourself? :)

As mentioned in a previous message (this still holds true now, after a few iterations), I didn't modify the core logic implemented by Statin, bar a few minor API surface points. I tightened up the Typescript typings quite a bit (no any, improved generics), I renamed functions/variables a lot to suit my personal learning / coding style, and I factored-out code into multiple files to ease maintenance and improve legibility (better visibility on build-time tree shaking too). I am really thankful for your original unit-tests, which I migrated to Vitest (including the Preact tests) as I absolutely love the instant compile/transpile + cache. I used Ava in another project, and I am quite familiar with Jest.

To summarise, so far this has been mostly an educational pursuit. I had been using Valtio as a "blackbox", and I love the DX. However I never really dug deep inside its guts (support for nested Proxy objects, fine-grained dependency tracking and minimized re-renders) ... very powerful techniques, but an order of magnitude more complicated than Statin!

PS: I implemented a barebones connector for Redux Dev Tools (the web browser extension) ... it kinda works but only makes sense for centralised object states (aka "store"), but not really suitable for granular state "atoms". Nonetheless the browser extension can help debug / track state changes, and "time travel" is a neat feature (rollback / forward state changes on a linear timeline). The former is dead-easy to implement (i.e. send signal updates as devtools events with JSON payloads that the extension can consume), however the latter is much more tricky due to state (un)marshalling / (de)serialisation (i.e. for example, nested signals loose their functionality so they break during time travelling).

Some references on the subject:

danielweck commented 2 years ago

Ah, another remark about unit-tests: Statin's error handlers capture except().toBe() -style assertions in some cases, which is a "gotcha" I failed to notice when I first migrated to Vitest (instead of Ava's t.plan() I am using my ad-hoc counters to verify the number of expected successful assertions). Thanks to Statin's synchronous approach, it was easy to migrate deep-nested unit test assertions (i.e. inside reactions / actions / effects callbacks) out of the closures, at the root of the test() functions 👍 (it simply becomes a matter of tracking count variables or such like, as there is a guarantee of synchronous execution)

danielweck commented 2 years ago

May I please ask you about Drovp's state management? (not open source, I believe)

Do you maintain a central external data store, shared via context down the component tree? Or do you keep state atoms completely outside Preact, and "sprinkled" across the "view model" as needed? (the reason for asking is a follow-up to my experimentation with Redux Dev Tools integration)

https://github.com/tomasklaen/statin#project-state

tomasklaen commented 2 years ago

I plan on opensourcing Drovp (or at least make the source available) once I feel like it's in a good enough state to do so.

State in Drovp is one big store object with everything in it initialized outside preact, and any component that needs something from it just calls useStore() which provides it via context.

But I'm doing a lot of stuff there that would definitely be considered a bad practice nowadays, like the whole state being composed of models which have a reference to it, so they each can have comfy actions related to them right there on their instance. For example:

class Operation {
  store: Store;
  id: string;
  state = signal<'queued' | 'pending' | 'done'>('queued');

  constructor(store: Store) {
    this.store = store;
    this.id = getUniqueId();
  }

  start = createAction(() => {
    this.store.worker.start(this);
    this.state('pending');
    // rest of the starting logic
  });

  delete = createAction(() => {
    this.store.operations.delete(this.id);
  });
}

class Operations {
  store: Store;
  all = signal<Operation[]>([]);
  byId = signal(new Map<string, Operation>());

  constructor(store: Store) {
    this.store = store;
  }

  create = createAction(() => {
    const operation = new Operation(this.store);
    this.all.edit(operations => operations.push(operation));
    this.byId.edit(byId => byId.add(operation.id, operation));
  });

  delete = createAction((id: string) => {
    // deletion logic
  })
}

// Constructing store
const store: Store = {};
store.operations = new Operations(store);
// ... other models

So then in code I can just have stuff like:

const Operation = observer(function Operation({operation}: {operation: Operation}) {
  return (<div>
    <h1>{operation.id} {operation.state()}</h1>
    <button onClick={operation.start}>start</button>
    <button onClick={operation.delete}>delete</button>
  </div>);
});

const Operations = observer(function Operations() {
  const {operations} = useStore();
  return <div>{operations.all().map(operation => <Operation operation={operation} />)}</div>;
});

It definitely doesn't provide the robustness of state debugging that something like redux would, but it's super comfy and fast to develop anything like this. And I've yet to encounter something that couldn't be debugged with a couple of console.logs :)

In redux, every time you want to change or add something to the state, it means messing with actions, action creators, selectors, dispatchers, connectors, mappers, ...

Drovp was originally made with react+redux, and I had to switch once I realized how much I hate working with it. It was also slow, and due to the amount of data Drovp is optimized for, state changes were causing frequent stuttering of the UI. Using immutablejs fixed it, but that just added one more inconvenience working with redux, and another 100kB dependency, just so that I can have state, which I don't consider a good trade off.

With statin, I traded robustness for comfort and speed of development.

tomasklaen commented 2 years ago

I tightened up the Typescript typings quite a bit (no any, improved generics)

I'd definitely be interested in pointers how to improve typings in statin.

By removing any, you mean the internal stuff right? I tend to make types facing users tight, but get a bit looser for internal things 😓.