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

Refactor: engine <-> client interface implementation #67

Merged
merged 87 commits into from
Apr 12, 2024

Conversation

eyelidlessness
Copy link
Member

@eyelidlessness eyelidlessness commented Apr 1, 2024

Closes #45, and addresses the broader set of goals behind that issue.

Initial writeup meta-commentary

If I did actually miss anything important in the writeup, it doesn't seem to have been important enough to resurface since. This definitely feels like more than enough for future archaeology.


I'm initially opening this as a draft with "[WIP]" in the title. It's very nearly ready and I want to make it visible now that it's so close.

TODO

There are a few last bits to wrap up:

  • Prevent nested client reactive object creation
    • Map writes to node.state.clientState.children to an array of the nodes' nodeId
    • Map that same array of nodeIds back to their corresponding node objects in the client-readable node.state.currentState.children
    • Ensure JSDoc and any pertinent call site interfaces clearly explain this behavior and the reasoning for it
  • Migrate remaining references to previous implementation of form instance state (and then remove it)
    • Consider whether ui-solid XFormsDetails component still has any value. If so, produce equivalent client-facing interfaces. (We will definitely be producing a subset of this—serialized representation of primary instance state; I'm just unsure if it'll be in this PR or a future iteration.)
    • Migrate ported JavaRosa tests
      • The Scenario class and related details. I am considering making this a separate package to emphasize the intent and fact that it is a client of the engine, just like any of the UI packages are.
      • Likely need to make a handful of type changes in the ported tests themselves
    • Migrate any other remaining tests pointing to EntryState or its descendants (a couple simple tests in ui-solid, quite a few in xforms-engine exercising XForms computation logic and reactivity)
    • Remove packages/xforms-engine/src/state (EntryState and its descendants), and all exports from there at the package entrypoint
    • Remove other assorted dependencies and aspects of implementation related to EntryState which will become dead code upon its removal (off the top of my head now: the @solid-primitives/set package, a couple of modules in packages/xforms-engine/src/lib/reactivity; possibly more)

I'll likely want to add even more detail after wrapping up the above TODOs, but I think I've captured many of the important details below.

But first: this is a lot. I'm writing this section last, before I open the PR... because I want to really recognize and acknowledge that this PR is huge. Not just in lines of code but the density of what's in those lines of code.

Writing everything below has re-emphasized that fact in my own mind. And I am quite certain I'm missing several important details which I'll want to update later.

I realize the hugeness of this PR is a burden, not to be taken lightly. I also think it's necessary. There are a lot of holistic and cross cutting concerns that came together in the design of #45 and everything it implies, and they would likely have been much less coherent without treating this as a singular effort.

I will say that I've also put considerable time and effort into making the commit history coherent, and trying to show the conceptual evolution from each of the foundational aspects into the set of changes as a whole. I highly recommend reviewing these changes commit by commit.

Okay, onto the details...

State: engine, client, current

To satisfy the interface goals in #45, this PR implements a tree of nodes corresponding to a form's primary instance state. Each node exposes a client-facing currentState object (and corresponding types), which is a read-only representation of the node's runtime state at any point in time.

Clients supply a state object factory on form instance initialization. This factory is used to construct a corresponding mutable object (clientState, internal to the engine) from which the client's currentState reads. Assuming a client's state object factory is backed by some client-specific implementation of subscribe-on-read reactivity, any writes performed by the engine to this internal mutable object are expected to propagate to that client's reactivity system and any pertinent reactive subscriptions.

To produce and maintain this clientState object, each node has a corresponding engineState object. This object is internally reactive—currently still using Solid, but this is expected to be an implementation detail encapsulated in the engine and opaque to clients (unless the client is also using Solid reactivity; more on that below)—and many aspects of XForms computations and semantics are built using this internal reactivity.

The engineState object itself is ultimately the source of each node's state. It is also notably derived from a specification object where:

  • Properties assigned a signal or similarly shaped getter/setter tuple are mapped to an engineState getter/setter property. Writes to those signals are reactively written to the same property in clientState (and reactively readable by clients as the same property on currentState). Reading such a property is also internally reactive to facilitate pertinent XForms computations and semantics.
  • Properties assigned a getter function (like a Solid memo, or any other thunk/zero argument function returning potentially reactive derived state) are mapped to a getter-only property. Any state changes which would update a reactive subscription to the provided getter function will, in turn, be propagated to the same property in clientState/currentState in the same way as signal-like state. Reading such a property is similarly internally reactive.
  • Any other property value (in neither a signal-like or memo-like shape) in a node's state specification is assigned as a static, read-only value to each state object.

That's a lot of words. Here's an example:

import { reactiveObject } from 'some-ui-framework';
import { createSignal, createMemo } from 'solid-js';

const { engineState, clientState, currentState } = createSharedNodeState(node, {
  value: createSignal('foo'),
  relevant: createMemo(() => computeRelevanceSomehow(node)),
  hint: null,
}, { clientStateFactory: reactiveObject });

engineState.value // get/set property, reads internally reactive to writes
engineState.relevant // get property, reads internally reactive to `computeRelevanceSomehow(node)`
engineState.hint // null, writes to this property will fail with a `TypeError`

clientState.value // reactively written with `engineState.value` whenever it's written by the engine
clientState.relevant // reactively written with `engineState.relevant` whenever it reactively updates in the engine
clientState.hint // null, never updated

currentState.value // getter for `clientState.value`, client-reactive
currentState.relevant // getter for `clientState.relevant`, client-reactive
currentState.hint // getter for `clientState.hint`, never updated

XForms computations and semantics

With a couple of exceptions, the intent in this PR is to preserve the same form behavior as existed prior to #61 (also restoring ui-solid reactivity which was temporarily broken in that PR). As much as possible, nuances of these existing behaviors have been clarified both in terms of abstraction boundaries and in many cases much more extensive JSDoc/other commentary.

The most notable known exceptions:

  • At least one repeat-specific bug I was aware of is now fixed: certain computations depending (even indirectly) on repeat position now behave correctly when repeat instances are removed. Specifically: this fixture originally taken from JavaRosa behaves as expected (when modified to use relative XPath repeat references, as is the behavior we're now targeting).

  • The selection behavior with itemset filtering discussed in What is the expected behavior when a selected select value is temporarily removed? #57 is left unaddressed in this PR. In general the interaction between selection and itemset filtering should be considered incomplete and likely buggy. Given the behavior is an open question, it felt reasonable to defer implementation of this functionality rather than reproduce an opinionated behavior we may not want.

  • When a field with calculate and relevant bind expressions:

    1. Is calculated (on load or update to dependencies)
    2. Has that calculated value overridden in a client (e.g. by user intervention)
    3. Becomes non-relevant
    4. Becomes relevant again

    Updated behavior: recalculate from the current state of the node's dependencies.
    Previous behavior: restore the overridden value from step 2.

    This previous behavior was inherited from Enketo (with excludeNonRelevant: true)—behavior which I had implemented because I understood it to be expected at the time. My understanding now, based on numerous conversations about pertinent spec details, is that the updated behavior is expected.

Changes to the runtime DAG

Generally speaking, this is now mostly deferred to the Solid reactivity implementation, which is itself a DAG. We no longer need to be concerned with dependency order, cycle detection, etc.

We do still need to:

  • Track dependency references (for the most part as XPath reference sub-expressions as identified in a form definition). Nothing about this has changed in this PR.
  • Resolve those references to their corresponding nodes in the instance state tree. This is now done dynamically, as part of the EvaluationContext interface, which each node implements.
  • Establish reactive subscriptions to those resolved nodes. This was something the previous EntryState implementation did rather haphazardly without a clear understanding of how to handle node- or expression-specific semantics, and it was quite incomplete. This is now part of the SubscribableDependency interface, which each node implements. There will likely be more work here around expression-specific semantics, but it has significantly improved node-specific semantics (responsible for fixing the aforementioned repeat bug, likely fixing a slew of others we don't yet have tests for).

It's quitely likely that most or nearly all of these remaining DAG responsibilities could be handled by more conventional reactivity if we decouple XPath evaluation from the XML DOM.

For now, all graph and dependency related reactivity is now isolated to the createComputedExpression function (which depends on nodes' implementation of the aforementioned EvaluationContext and SubscribableDependency interfaces). This function builds a reactive memo, evaluating a supplied DependentExpression (introduced in #14, subject of much recent discussion when reviewing that PR), and re-evaluating the expression reactively as described above.

XForms semantics implementation details

Along with moving more of the DAG logic into the reactivity implementation, quite a lot of the implementation aspects of various XForms computations and semantics are now much closer to the way they'd be conventionally and/or idiomatically built with reactivity.

  • Each node's readonly, relevant, required are basically implemented as a small wrapper around createComputedExpression.
  • createValueState defers to those reactive relevant and readonly getters, and uses createComputedExpression to handle calculate where present
  • createNodeLabel and createFieldHint also wrap createComputedExpression for ref/itext translations and <output>s, and produce the client-facing TextRange interface as a memo around that
  • createSelectItems is a wrapper around createComputedExpression to resolve dynamic <itemset>s; select item labels use the same underlying logic supporting createNodeLabel (for both static <item>s as well as dynamic <itemset>s)

Value types

The aforementioned createValueState handles general XForms semantics around reading, writing and calculating values. It also introduces a ValueContext interface (which both current value/field node types implement), deferring to each node type to handle its own runtime value representation. For example, SelectField decodes the XForms string value to SelectItem[] and encodes those selected items back to an XForms string representation. It's easy to imagine how we'll expand this to similarly handle other arbitrary data types, regardless of their structural complexity (such as geo).

Solid-specific client details

There are a couple of special cases made for Solid (client) <-> Solid (engine) interaction:

  • A separate Solid-specific xforms-engine build is produced which externalizes the solid-js package as a peer dependency, which ui-solid (or any other hypothetical Solid client) must resolve when importing the engine. This is a common practice in the Solid community, and necessary for our usage, because bundling Solid in a transitive dependency causes conflicts where Solid itself expects to have global singleton state.
    • Related to this, any downstream package which also uses Solid must ensure it consumes this Solid-specific build. For our usage with Vite, that means setting viteConfig.resolve.conditions: ['solid', ...]. This has tripped me up repeatedly (and it very well may have been the reason we had earlier issues with the SUID package[s]). We should think about how best to document this, as Solid has been gaining popularity, and the prospect of some part of the web-forms stack being consumed in a Solid stack will likely become more realistic in the foreseeable future.
  • Dev mode in ui-solid currently references the xforms-engine source rather than its build, as a convenience for iterating on both packages simultaneously. It may be possible for ui-vue and other clients to be set up similarly if we think it would be beneficial. A case could also be made that we should revert this, because consuming the engine's build product will help us catch issues well ahead of any releases.
  • A special case is made in the engineState -> clientState flow, which is treated as a noop if a client supplies Solid's createMutable as its reactive state factory (detected as an equality check, which would likely fail without the Solid-specific build). This exception has been reverted. I had previously considered such a special case safe, but I found it harder to predict nuances of reactive behavior in other clients with it in place.

…locks

See: https://github.com/orgs/community/discussions/16925, specifically:

> Update - 14 November 2023
> […]
> The initial syntax using e.g. **Note** isn't supported any longer.
Allow `ok` property, make methods unbound, export types
It’s astonishing how hard it was to figure out that you can even do this with the intentionally broken jsdom implementations of `Blob` and `File`.
interaction with the engine/client API’s OpaqueReactiveObjectFactory.
None of the node classes are *actually* implemented yet, but they all have to exist to satisfy the various hierarchic aspects of the interfaces. After several attempts to do more for any one given node type, this approach feels like a much better foundation to build each node’s implementation.

There are also some hints at forthcoming implementation details ( `EvaluationContext` and `SubscribableDependency` internal interfaces are concerned with establishing reactive computations)
This is based on the recognition that the pertinent Solid reactive primitives align closely with the semantics of standard JS property descriptors. Ultimately, what we end up with is:

- Each node’s state is “specified” by an object defining the state’s property structure
- Each state property is either:
  - mutable: specified with a signal-like value, mapped to a get/set property backed by that signal-like value
  - computed: specified with a thunk/accessor/memo-like function, mapped to a get property backed by that function
  - static: specified with any *other* value, mapped to a non-`writable` property assigned that value
- Each node’s state (internal) *types* are derived with the same mapping logic

The appropriate derived state types will in turn be checked for **assignability** to their corresponding client-facing `node.currentState`.

- - -

This commit is a second iteration on this set of functionality. The earlier effort (now rebased out of this branch) produced objects with roughly the same characteristics. Where it differed:

- The input types were much less ergonomic in actual usage: generally speaking, each node had to perform its own version of the state/property descriptor mappings described above.
- Despite doing less work, the actual state initialization implementation was more complex because it was introspecting on property descriptors themselves rather than on simpler signal-like and thunk types.
… and a couple minor improvements to select/string node interface types
1. Reactivity doesn’t work at all (in the engine’s implementation of the interface)
2. ~~Even if it did, something causes a stack explosion passing `createMutable` as the actual state factory~~ (a minimal partial fix for the stack explosion is now in, but…) In the one place where it is wired up (form languages), effects and other downstream reactivity don’t seem to propagate out to the ui-solid package.

Otherwise, I’m reasonably certain that this is otherwise a faithful migration of the whole Solid UI to the new APIs.
1. Include a Solid-specific build with Solid itself as an external/peer dependency
2. Careful attention to `exports` order. Notably, this order matters **regardless of which `conditions` are specified in the client’s Vite config! And specifically, the `solid` condition (which Solid’s own Vite plugin specifies in its config modifications, but we also specify it in ui-solid for good measure) must come before **all of the others** which might be matched in a Solid environment. We may still need to revisit this order! In any case, I’ll be doing a quick spot check with Vue after this commit.
3. Restore ui-solid passing `createMutable` so there’s actually reactivity to be had in the first place.

Also of note, only one reactive behavior has been verified working: changing a form’s active language. It doesn’t yet update any downstream state (i.e. translations, which it couldn’t yet anyway, as all the `label` state properties are currently hardcoded to `null`). But it can be observed updating the language menu, which has been updated to be reactive to `root.currentState.activeLanguage`.

There’s still more to go, but this at least proves that the most fundamental assumption behind a configurable `OpaqueReactiveObjectFactory` still holds.
This will be temporary if we decide we want more internal reinforcement that our build process works as intended.

But it’s worth noting that it’s idiomatic/typical to publish and depend on source in the Solid community, and the previous step to make solid a peer dependency in ui-solid is already a significant step in this direction.
Conceptually, this introduces several pieces of state handled uniformly (or mostly so) by `DescendantNode`:

- `reference`
- `readonly`
- `relevant`
- `required`

Concretely, only the first of these is fully implemented. And that full implementation emphasizes the “mostly so” above: all nodes compute their `reference` by concatenating their parent’s reference with a suffix.

For all node types except repeat instances, the suffix is a named child step, where the name is the node’s own name.

For repeat instances, the suffix is a position predicate, where the position is its current index in the parent range’s reactive children state, plus one (because position predicates in XPath expressions are one-based). There’s some additional nuance about the reactive computation, detailed in the comment where that reactivity is set up.
Given an `EvaluationContext` and a `DependentExpression`, produces a thunk returning its computed value (in the type specified by the `DependentExpression` itself). This is laying groundwork for numerous computations which will either use it directly, or will build additional semantic abstractions on top of it. Actual reactivity will be implemented in a subsequent commit, and essentially “bring to life” the various computations utilizing it.
- Handles these bind expressions consistently for all nodes.
- Handles inheritance per spec.
- As with previous commit, presently defers handling reactivity.

(This commit has been amended. Previously it made a special case where nodes with a `calculate` bind expression would be treated as implicitly `readonly`. After a quick chat on this and related behavior, it’s been rolled back for now.)
… and preserve previous state, which is restored if the field becomes relevant again.

This restores behavior previously implemented in `EntryState` (and its value node descendants), with one exception:

1. Given a value node with `calculate` and `relevant` bind expressions
2. If the node’s value is calculated
3. If then the client/user manually set a value
4. If some other form state change causes the node to become non-relevant
5. If another form state change restores the node’s relevance…

… the `calculate` is re-run, discarding the previous client/user-provided value
- Satisfies the client `TextRange` and related interfaces
- Suitable for Group label use
- Suitable for StringField and SelectField label/hint use
- Suitable for SelectItem label use
- Reactive to current form active language (though that reactivity will be moved in a subsequent commit generalizing the reactivity approach)
- Static items’ values are computed upfront
- Dynamic itemsets are computed with the expectation the items themselves will be made reactive by `createComputedExpression`
- All items’ labels are computed with the same primitives defined to support Group/StringField/SelectField labels/hints
… to client interface, to help with type narrowing. This is a first pass, to examine its downstream improvements (for now in `scenario` and `ui-solid`; I’d include `ui-vue` but that branch is also downstream). All nodes currently get a `nodeVariant: null` placeholder, and individual variants will be added in a subsequent commit so their specific narrowing improvements can be seen separately.
Copy link
Contributor

@sadiqkhoja sadiqkhoja left a comment

Choose a reason for hiding this comment

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

Done reviewing the src of xforms-engine. Overall this is great 👍

General comment: One pattern that makes me nervous is passing this to sub-routines in the constructors.

class Person {
   yearOfBirth: number;
   age: number;

   constructor(private name: string) {
      // this will fail because calculateAge depends on yearOfBirth
      // and method definition doesn't convey that information
      this.age = calculateAge(this);
      this.yearOfBirth = new Date();
   }
   
   calculateAge(p: Person) {
       // And if I am writing this method, I have no idea if p: Person is
       // fully initialized or not.
       return 2024 - p.yearOfBirth;
   }
}

@eyelidlessness
Copy link
Member Author

General comment: One pattern that makes me nervous is passing this to sub-routines in the constructors.

I agree, and again this speaks to some of my discomfort with classes for some of this. I will say that I've tried to generally to make sure the input types to those sub-routines are narrower than the types of each node's this, to at least help make it easier to spot when an incomplete instance construction occurs. Off the top of my head, the main exception to this is in createSelectItems, which I'd be happy to revisit if that helps.

I’m making this change because I can’t honestly come up with good answers to the questions in [this review comment](#67 (comment)). But I’m cautious about this change, and want to make note of it for forensic purposes in case we find a need to revisit it.

I distinctly recall encountering specific edge cases warranting the previous iteration. I cannot for the life of me recall what they were or how I would go about reproducing them. And I weakly suspect that even if I could, they will have been addressed by later changes to children state (i.e. moving it earler in each parent node’s construction, as part of 8b46fcd.
Copy link
Contributor

@sadiqkhoja sadiqkhoja left a comment

Choose a reason for hiding this comment

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

I see few test files in src folder of common and xpath packages, we should move them to the test folder either in this PR or later.

After the prototyping discussed in #73, I now doubt this is the right direction to take
Per excellent suggestion from @sadiqkhoja.

Also significantly expands on JSDoc of what removal logic is performed, where, in what order, and why.
Neither have been in use since the introduction of `createSharedNodeState` and the state types derived from each node’s `StateSpec`. I had removed these in a WIP commit, but somehow they slipped back through in a rebase.
…methods

Note: there’s potential for an additional micro-optimization, skipping encode/decode entirely in `createValueState` when these functions are recognized as `identity`. I skipped this for now because, literally, I am too tired to do more code today.
Copy link
Contributor

@sadiqkhoja sadiqkhoja left a comment

Choose a reason for hiding this comment

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

🚀

@eyelidlessness eyelidlessness merged commit 30ea4b9 into main Apr 12, 2024
72 checks passed
eyelidlessness added a commit that referenced this pull request Jul 19, 2024
As discussed in review, it’s highly unlikely this is actually needed anymore!

Brief backstory context in case we ever have reason to doubt this:

0. The WHO VA form, which I’ve used as a frame of reference for many things, uses two references to `null` that don’t resolve to anything. The inferred intent is to treat `null` as a keyword, just as it would be in languages with the Billion Dollar Mistake.
1. In early prototyping (pre Web Forms project), the WHO VA form’s references to `null` produced errors where there was assertion logic checking that expected dependencies were found.
2. Early Web Forms work produced the same, for a time at least producing errors in the console. This produced a lot of distracting noise that made it harder to identify actual implementation errors in dependency resolution.
3. Subsequent improvements to dependency resolution have been successful enough that we’ve even eschewed logging when a dependency lookup doesn’t resolve (though I sometimes add it in local dev for testing/validation), at least as of #67.
4. As of #135, dependency resolution was expanded to be form-wide and match all possible nodes; there’s no case where a `null` reference **should** match a node and won’t.

I can imagine these potentially useful followup changes:

1. On parse, identify any path sub-expressions which **will never** resolve to any node in the form.
2. Short circuit lookup of such never-resolvable paths. There’s no sense walking the full form tree for something we know we won’t find!
3. Potentially warn when such never-resolvable paths are found. This wouldn’t be particularly useful for the `null` NameTest specifically (where the intent at least in WHO VA is clearly a null reference), but it could be useful for catching typos and other mistaken references like hierarchy confusion.
eyelidlessness added a commit that referenced this pull request Jul 22, 2024
As discussed in review, it’s highly unlikely this is actually needed anymore!

Brief backstory context in case we ever have reason to doubt this:

0. The WHO VA form, which I’ve used as a frame of reference for many things, uses two references to `null` that don’t resolve to anything. The inferred intent is to treat `null` as a keyword, just as it would be in languages with the Billion Dollar Mistake.
1. In early prototyping (pre Web Forms project), the WHO VA form’s references to `null` produced errors where there was assertion logic checking that expected dependencies were found.
2. Early Web Forms work produced the same, for a time at least producing errors in the console. This produced a lot of distracting noise that made it harder to identify actual implementation errors in dependency resolution.
3. Subsequent improvements to dependency resolution have been successful enough that we’ve even eschewed logging when a dependency lookup doesn’t resolve (though I sometimes add it in local dev for testing/validation), at least as of #67.
4. As of #135, dependency resolution was expanded to be form-wide and match all possible nodes; there’s no case where a `null` reference **should** match a node and won’t.

I can imagine these potentially useful followup changes:

1. On parse, identify any path sub-expressions which **will never** resolve to any node in the form.
2. Short circuit lookup of such never-resolvable paths. There’s no sense walking the full form tree for something we know we won’t find!
3. Potentially warn when such never-resolvable paths are found. This wouldn’t be particularly useful for the `null` NameTest specifically (where the intent at least in WHO VA is clearly a null reference), but it could be useful for catching typos and other mistaken references like hierarchy confusion.
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.

Initial read interface design between engine and UI
3 participants