Chore (xforms-engine): finish reorganization of parse-related file system structure #213
Conversation
All of the following are moved, preserving their current structure: - XFormDefinition - XFormDataType - XFormDOM - body/** - expression/** - model/** One or more subsequent commits may reorganize further.
Keeping these colocated with their containing objects seems to make less and less sense over time. What makes more sense (I think): These define the form’s core/fundamental computations[^2]. Together, they effectively describe the engine’s runtime behavioral aspects of a form[^3]. It makes sense to be able to see that, at a glance, as a whole. [^1]: Currently excepting `TextChunkDefinition` and _its subclasses_. I want to do these separately to see if this really feels right (for them, overall), with an easy opportunity to roll back to this commit if it feels right for this subset but not for those text aspects. [^2]: for the most part! we may want to do another pass to see if there are other candidates that should be handled the same way, or other ways of defining them such that we can colocate all such expression parsing together. [^3]: Apart from client-facing state change
…classes Having taken some time to sit with this (separately from the other DependentExpression cases): 1. I think it does make sense! It also makes the connection between these “chunks” and their computations much more obvious. 2. The name changes also feel like an improvement! I especially think distinguishing “static” from “literal” is valuable here (i.e. it is consistent with the value being a Literal in terms of XPath grammar as well) 3. It’s probably a good idea to keep this as a separate commit, so the reasoning and impact can be tracked together
…classes This feels like a more helpful grouping than: - mixing the abstract/concrete together - grouping the text expressions (which now group well by prefix anyway)
I’m kinda iffy on this name, BindExpression might be fine… but I think that would be potentially ambiguous in a way that could matter. There is at least one important “bind expression” which is not addressed by this class, and shouldn’t be: `nodeset`. My gut instinct is that the shorter name would be misleading and could cause mistakes down the road. Also renamed: `forExpression` -> `forComputation`. The former felt weirdly more redundant, and this makes a bit more semantic sense anyway. + minor comment cleanup
…lasses In hindsight, this should have been in a `model` subdirectory when it was introduced!
Nothing left TODO! This reorganization task is complete.
…utine It’s really a wonder this hasn’t surfaced before! And it’s unclear why the parse module reorganization is what finally did surface it. But now’s as good a time as any to let a forcing function force another small improvement.
|
|
I should mention: this shouldn't result in any meaningful change for clients. It does rename some internals which are observable at runtime in clients, so it could be construed as a "breaking change". But I don't believe any of the affected types are exposed as directly client-importable. And anything transitive a client could access (by way of a node's If we were post-1.0.0 I'd want to discuss the semver implications of this. I don't think it's worth even a changeset in our current release posture, but I do think it's worth mentioning so |
I have verified this PR works in these browsers (latest versions):
I haven't done any manual verification at all, apart from running
yarn workspace @getodk/xforms-engine test:typesa zillion times as I made incremental changes. If anything is broken that wasn't caught by that, we have much bigger problems. And even if so, there's a lot of downstream CI to alert us if that's the case.Edit (before I even had a chance to open the PR!): turns out downstream CI did alert us to an issue. More on that in the change description below.
What else has been done to verify that this works as intended?
I relied heavily on VSCode to do nearly all of the work.
Why is this the best possible solution? Were any other approaches considered?
We agreed that this change was desirable some time back. I finally decided to finish the task, because it was beginning to irritate me introducing more new parsing functionality with this partially done.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
I don't believe anything in this change is a regression risk which won't also be caught by CI.
Do we need any specific form for testing your changes? If so, please attach one.
N/A
What's changed
This completes a task, begun in beb4d38 (#154), to reorganize engine parsing logic together in a coherent way. In more detail:
src/parsewas moved there in its current file system structure, with no source code changes apart from updating import paths.DependentExpressionsubclasses to reflect their expression-ness, and to restructure those together. This was done in several steps, as I wanted to feel out the implications without getting too deep into such a broad change.DependentExpressionclass was made explicitlyabstract. I expected to do this when the concept was introduced, but I deferred commiting to it until we had more usage in place to determine whether it made sense to proliferate subclasses for every usage. (It does!)All of the above were done with heavy reliance on VSCode and the TypeScript language server to make sure the appropriate imports and name references found their way to all affected call sites. Finally, one issue did arise in CI that didn't show up in type checks:
BodyDefinitionand group/repeat element definitions resulted in some runtime errors parsing repeats. Importantly, this change did not introduce the problem, it only caused it to finally be surfaced. I don't entirely know why that is, but we would likely have encountered issues with it eventually anyway. I went ahead and picked a narrow fix: assigning abodyreference to each body element definition, updating these recursive child constructor calls to reference the same logic on aBodyDefinitioninstance method rather than the static method which precedes it. A more thoughtful fix would likely involve something more like the child construction logic insrc/instance/children.ts, but that felt like more of a scope stretch.I would recommend reviewing this commit by commit, I think that'll at least make it easiest to see which changes were most/least assisted by TS/VSCode. And I think the commit notes will give some insight into why some of the steps were taken as they were (which will hopefully be helpful for future git history quests as well).