-
Notifications
You must be signed in to change notification settings - Fork 12
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
Split xforms-engine
/ui-solid
packages, update naming of other packages
#61
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also, fix package.json > repository > directory
Also: - Fixed a few missed `tree-sitter-xpath` references along the way. - Fixed an errant comment trailing off
Subsequent commits will split out the current UI aspects into a separate `@odk-web-forms/ui-solid` package. An attempt will also be made to move fixtures which are generally used for demo purposes into a shared space that any UI client can reference (with the Solid client being a reference implementation for that).
…ild to the engine aspects
This was meant to be removed after the Vue package init. It was testing whether ESLint’s `globals` rules really do treat `globalThis` as read-only (they don’t)
Primary purpose of this commit: Move all aspects of UI, as implemented with Solid, to the `@odk-web-forms/ui-solid` package Necessary to support that: Move the JavaRosa-derived test form DSL into the `@odk-web-forms/common` package, so it can be shared across packages (currently used in both `ui-solid` and `xforms-engine` Also necessary to support that: a whole lot of tooling-related churn. Some of this is hard to recall, after a few rounds of trial and error and finally wrapping up with some stuff much simpler than it started. To the best of my recollection… - Vite and Vitest each issues around: - `web-tree-sitter`’s particular (and particularly odd!) CommonJS module structure getting even more confusing to import in various environments - `web-tree-sitter`’s references to Node modules getting mangled in weird ways when it and `@odk-web-forms/xpath` become transitive dependencies (`xpath` -> `engine` -> `ui-solid`) - `@odk-web-forms/xpath` parser init breaking as a transitive dependency, due to differences in how the `tree-sitter` and `tree-sitter-xpath` WASM resources are referenced between build phases - Our derived version of the Solid testing library behaving differently once moved to `ui-solid`, for reasons still unclear - Updating both of those was already on the agenda, so I gave that a go. That revealed further issues with: - A bunch of stuff either internal to Vitest throwing import errors or other build tool errors in various test/build/downstream contexts - Apparent side effects of merely having anything `@testing-library/…` installed, at all - Our `suid` vendor package, which itself had worked around previous issues in previous Vitest versions, causing problems anew just by existing as such a workaround The good news is that resolving **almost all of these** has resulted in simplification! - The `suid` vendor package is no more - The `@odk-web-forms/xpath` package now bundles `web-tree-sitter` (JS/WASM) and `tree-sitter-xpath` (WASM), which (TODO) should allow us encapsulate init and stop pushing that responsibility to downstream packages - A variety of Vite config stuff is either no longer necessary or simplified - Did I mention Vite/Vitest aren’t out of date now? Lastly, a known issue: all client reactivity is temporarily broken due to the xforms-engine/ui-solid split. The root cause of this is known and understood (it’s a common symptom of implicitly mixing multiple Solid reactive roots), but it’s moot: it will be fixed in the next branch/PR, where we incorporate the new client interface design/implementation which accepts an explicit client reactive factory.
…-web-forms/common`
Some of the lib stuff really is still library-ish. But now the rest is actually just the source implementation
eyelidlessness
force-pushed
the
reorganize/package-naming-structure
branch
from
March 12, 2024 16:57
4d7a32a
to
8990744
Compare
sadiqkhoja
approved these changes
Mar 14, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the PR commit by commit, all look reasonable.
This was referenced Apr 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #40, among other things. I'll update the PR notes with more detail tomorrow, but b9bc5ad likely covers all of the most important (and gruesome) details.