Vue client

[sadiqkhoja]

Closes #34

What's in it

• Vue components to render Groups, Repeats and Text questions
• Uses PrimeVue to render material components
• Component tests
• An e2e test to render all test forms using brute force

[eyelidlessness]

I took a ton of notes Friday and today as I walked through this PR, took time to understand the behavior and approach, and thought through some implications about what's downstream from here.

This review reflects part of those notes: pretty much everything that I could reasonably address inline. I want to post it now so there's some feedback visible going into tomorrow.

Also tomorrow, I'll go through the rest of my notes to address a few higher level/cross cutting concerns. Mostly these have to do with:

• Naming
• Code style, and automating more around that (because I'm sure none of us wants code style to be an ongoing focal point of review)
• Generally addressing errors and incomplete functionality as minimally as possible (without going under)

[eyelidlessness]

Error conditions, incomplete support, etc.

I don't want to over-burden this PR by extending scope, but I do think some minimal notion of these should be in scope:

• Some way, even if it's not yet user-visible, of surfacing error conditions that occur. This is discussed in some detail on the E2E test, but I want to call it out here as a general thing I'm looking for.

• Some way that is user-visible (but absolutely minimal for now) of identifying aspects of a form which aren't yet supported. Again this is discussed a bit inline, but I'm thinking mostly about pending support for selects (Vue UI: Select_one and Select_many #59). I know that'll come in due time, but I'm more interested here in:

  • Not establishing precedent for adding known-broken behavior, when it can reasonably be flagged as incomplete instead.

  • A designated place that we can come back to, to expand on how we actually want to convey missing functionality to users when we address that as explicit user-facing behavior.

Naming: Odk prefixes, prefixing generally

I’m curious about the rationale for Odk prefixes. Maybe there’s thinking behind them that I haven’t grokked. But my thinking is that they seem superfluous: isn’t approximately everything in this codebase “ODK something”?

More generally: when I started on web-forms, I found domain-/spec-specific prefixes helpful in calling out where a given concept had a formal basis that could be referenced, or where there were derivative concepts within that space. This still feels right in, for instance, areas of the xpath package, where there are concepts built around/layers built upon those domain-/spec-focused concepts. It also started to feel superfluous in what’s now the xforms-engine package—where nearly everything could have an XForms prefix. I have mixed feelings about its application in ui-solid: there too a notion had been forming around layers built up from XForms* to more user-facing concepts.

While we’re on the topic, I do find more cognitive overhead when using a PascalCase convention that lowercases parts of an acronym. In other words, OdkFoo tends to be harder for my brain to recognize without extra thought than ODKFoo.

Naming: [Odk]Repeat

I don't know if we would want to inherit the terminology that evolved out of the client interface design, but RepeatRange does seem more clear to me than Repeat. The terminology is especially ambiguous because it's common, colloquially, to refer to repeat instances as the unqualified noun "repeat" (and those do have the more specific name [Odk]RepeatInstance here, as I'd expect).

Naming identifiers: words and shorthand

It’s probably not a surprise given the state of the rest of the codebase, but I tend to prefer longer names—using whole words, letting names help with self-documentation—over briefer, abbreviated names. Besides packing more information into a few more characters, this convention is also generally more idiomatic in the language (and I did lean more toward shorter names when I worked in Clojure, where that’s the idiom). Some exceptions make a lot of sense, and they also tend to be idiomatic. For instance i for “index” in for loops or various iterative APIs, or acc as a reduce's accumulator, usually have instant recognizability.

Otherwise I would like to consider a general preference for names—for modules, variable bindings, types, and so on—which are descriptive and use whole words unless there’s a common convention for abbreviating them.

Lint rule to increase safety

This isn't an issue in any of the PR's changes, but it came up while I was trying things out in review. I was surprised that, by default, it's not an error in Vue for template tags to reference components that don't exist/aren't in scope. I think we should enable this rule so that it does become an error:

+ 			'vue/no-undef-components': 'error',

Code style/formatting: automation

I don’t know how much justification this will need, so I’ll try to keep that brief. To be clear, my goal with settling on common code style and formatting is to reduce the mental overhead that comes with inconsistent code styles and the mistakes that often get missed due to that overhead. It’s not (to me) a matter of preference—in fact, quite a lot of the auto-formatting in use is fairly different from my own preference—but about eliminating the need to even think about code formatting as much as possible, so we can spend those mental cycles on the much more important work we’re doing. With that said…

There are a few code style issues that are not currently being automated as well as I’d hoped, and I’d like to try to address them sooner rather than later to hopefully avoid big reformatting commits in the future. Those make navigating commit/project history a huge pain!

Part of this involves adjusting some of the project configuration. I also want to see how much we can align on editor setup so that most formatting is applied consistently on save. If that doesn’t make sense, maybe we can look at git hooks or some other automation mechanism so we can mostly turn our focus away from bike shedding how code is formatted, and focus as much as possible on what we’re achieving with consistently formatted code.

Config

I noticed that VSCode is not auto-formatting on save as I’d hoped in .vue files. I think making this change in .vscode/settings.json will get us closer to that:

    "[vue]": {
-     // Vue files are ignored in Prettier. Some formatting **will** be applied
-     // on save by ESLint (via `editor.codeActionsOnSave` settings above)
-     "editor.formatOnSave": false
+     "editor.defaultFormatter": "dbaeumer.vscode-eslint",
+     "editor.formatOnSave": true
    },

I also noticed that when auto-formatting is applied, it isn't applying whitespace rules consistently. I would like to add these to the Vue section in eslint.config.js:

+ 			'vue/html-comment-indent': ['error', 'tab'],
+ 			'vue/script-indent': ['error', 'tab'],

I also noticed several components unexpectedly have empty <style> blocks. I'd like to prevent empty blocks in components generally:

+ 			'vue/no-empty-component-block': 'error',

Editor setup/other automation mechanisms

Auto-formatting is only so good as its auto-ness. From what I can tell, auto-formatting that is working correctly for me locally isn't applied consistently throughout the PR.

And again, I don't know how much effort I should put into justifying this, but I'll say that these discrepancies probably account for about 10% of the time I've spent reviewing this PR. Both in terms of mental overhead, and in terms of frequent mechanical steps to revert unrelated local changes as I'm exploring the changes on the branch and trying things out.

We can't (and I wouldn't want to) enforce a particular editor and its configuration, but if there's willingness... I've found it really helpful to install the VSCode extensions recommended in, well, .vscode/extensions.json (and if you use VSCode, there's usually some UI prompt to install these when opening a project with that file).

If that doesn't work (and maybe we should consider this anyway/in addition), we can explore options like:

• A git hook to apply formatting on commit
• An additional CI check to block when yarn format and/or yarn lint --fix produces uncommitted changes
• Something[s] else? I'm all ears

Code style/formatting: a few bikesheds now…

… so hopefully we have fewer in the future.

• I didn't see a lint rule for this, but I'm hoping there is one: can we generally prefer <style scoped> over unscoped component styles? There may be reason to make exceptions, but I think they're likely to be rare.

  This would have been a benefit while I was exploring how selects support (Vue UI: Select_one and Select_many #59) might play out. I got very confused about why PrimeVue's RadioButtons were all showing a not-allowed cursor, and it turned out that the styles from OdkQuestion.vue were being applied despite being in another component. Evidently RadioButton uses a common technique for progressive enhancement to style built-in <input type="radio">, where the styles are applied to an overlay element with pointer-events: none; it also sets those inputs to readonly (at least when they're controlled?), which led to this confusion where the radio buttons were definitely user interactive but the unrelated styles suggested they shouldn't be.

• Currently (with appropriate editor support), imports are auto-sorted. This fails when there are blank lines between import statements.

  Personally, in the past I've been a fan of grouping imports (such as is common in Python, e.g. [stdlib imports]_ [dependency imports]_ [local imports]*), and if that's something we want to investigate I'd be open to that (with the caveat that I wasn't thrilled with JS/TS tooling solutions for this last I looked).

  Otherwise, I'd like to avoid such blank lines between imports where possible so we can benefit from existing auto-sorting (which, while not my preference, at least tends to be consistent and predictable).

• In .vue <template> blocks (and in markup generally), I find it really hard to read long opening-tag lines. I'm not sure whether reverting the vue/max-attributes-per-line change is the best solution here, but I'd like to agree on a way to keep those lines shorter if possible.

  (Side note: in general, long lines is a place where I've frequently found myself reach for // prettier-ignore.)

• Not something that needs action in this PR, but just an observation about template tags and prop interpolation: I find the convention of using a Record<ClassName, boolean> to toggle an element's classes super useful. I'm calling this out because I think it's a big exception, where generally inline objects in props tend to make render templates harder to read (although that can often be counteracted with shorter lines, especially if there's a hard prop-per-line rule).

• I can take another look at this, but I'd like to see if we can fully automate including file extensions on imports. This has been a bit controversial in TypeScript in the past, but it's generally clear now that every environment is moving toward either requiring or allowing explicit file extensions. Especially since some environments require extensions: being flexible about supported environments is part of my motivation.

A couple visual style tweaks

I know we're not going for full design fidelity here, but I want to reiterate some concerns about:

• Top-level groups' [Odk]Panel appearance. These look like cards stacked on the form's base card, and it's unexpected. If I'm not mistaken, I think nested groups look close if not exactly like the design. Can we just eliminate the top-level distinction?

• Repeats, if I'm not mistaken, currently have the same styles as groups. Again, I don't think they need to have full fidelity with the mockups, but I've (ahem) repeatedly been tripped up by their similar appearance to groups.

[eyelidlessness]

Oof, one more naming thing I forgot. Can we standardize on .test.ts (or .test.{ext}) suffixes for test modules? I also want to make this change for the inherited tests in the XPath evaluator. It's a small thing, but it would be one less thing to think about.

[sadiqkhoja]

I have addressed all the inline comments, for some I am looking for some suggestions.

For the general comment, I have made the following changes:

• Removed ODK prefix from the all but one component
• Included suggested lint rule
• Renamed test file from *.spec.ts to *.test.ts
• Changed the order of and <script>

  I like this idea for consistent format and style:

  An additional CI check to block when yarn format and/or yarn lint --fix produces uncommitted changes

[eyelidlessness]

This is a great first big push on moving the web forms UI to Vue.

I had previously discussed additional changes I was interested in making, but I'll be glad to move forward with this as-is.

[eyelidlessness]

Appreciate the followup changes. I don't have any additional feedback.

Seeing the E2E changes, I think it makes sense for now. I'm definitely looking forward to finding out how we'll address error conditions in E2E tests long term, and diving into the work to support that both in the engine and UI.

I did a quick gut check running locally to validate: all feels ready to merge from my perspective. Great work, thanks @sadiqkhoja!