-
Notifications
You must be signed in to change notification settings - Fork 28
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
Replace harness instruction page rendering #434
Replace harness instruction page rendering #434
Conversation
cbd29da
to
fd54347
Compare
tests/resources/vrender.mjs
Outdated
@@ -0,0 +1,906 @@ | |||
/** |
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.
This file is pretty mind blowing! This ain't everyday JS, this is shiny stuff. I find myself wondering how you did this!
I think, and correct me if I'm wrong, that this is a virtual DOM implementation to be used for rendering the tests, and I believe from our conversations that we wouldn't use it in the app, it would be confined to the aria-at repo.
Which of course, the thought of having to come in here to fix something is making me sweat slightly, haha. It's frankly over my head right now. So my question is, is there a way to avoid having to maintain this ourselves? I'm sure the reasoning is solid but I was hoping you would take a little time to help me grasp it.
Finally, I was wondering if there's a good way to test this file specifically. How do I put it through its paces so I feel like I gave it the review it deserves?
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.
to echo @alflennik's comments, could you share your rationale for writing a custom implementation?
Any reason you didn't lean on a more commonly used dependency such as lit-html or snabbdom?
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.
I think, and correct me if I'm wrong, that this is a virtual DOM implementation to be used for rendering the tests, and I believe from our conversations that we wouldn't use it in the app, it would be confined to the aria-at repo.
Yes. aria-at-app will use the javascript object produced by instructionDocument
. aria-at itself renders that same object with this virtual DOM implementation.
I'll use the rest of this comment to reply to Seth's added point.
to echo @alflennik's comments, could you share your rationale for writing a custom implementation?
Any reason you didn't lean on a more commonly used dependency such as lit-html or snabbdom?
My thoughts on priorities in this PR
- Add a rendering library
- tracks state in js objects and not dom
- output a document, a js object, representing the structure, text, and event handling that can be transformed into dom by the library consumer (e.g. aria-at-app)
- Maintain behavior
- Focus on the page header on first render
- On keyboard event on a checkbox focus an adjacent checkbox
- Change classes when submitting results when some fields are invalid, focus the first invalid form input
- Keep a low probability of adding bugs in this change
- Produce the same html structure
- Minimal external dependencies
- Easy to turn instructionDocument into the current html shape
- Many adjacent elements represent a semantic section without a parent element
- Fragments like in React give an easy way to produce a html structure with semantic and not structural sections
- Fragments help break up long sequences of adjacent elements into helper functions
- Ease of changing the rendering in the future after this PR
- Such as supporting new test features
dom libraries comparison
-
LitHTML
- Adds one dependency
- Cannot produce the same dom
- Inserts html comments at dynamic content locations
- I find it hard this would have an impact but conservatively I want to avoid it
- Supporting focus behavior
- Option: Use components and their update hook
- Components are web components (custom elements)
- This would put a change into the html structure
- Option: Manually find the right element after rendering without components and focus it
- Option: Use components and their update hook
- Inserts html comments at dynamic content locations
-
Snabbdom
- Adds a minimum of one dependency (components are a separate dependency: kaiju)
- Does not support fragments support array of elements without needing a wrapper div snabbdom/snabbdom#560
- Option: change the html structure, add wrapping elements
- Option: Use the spread operator (would not work with components)
- Option: Use less helper functions and repeat many elements more manually (would not work with components)
-
vrender
- Internal dependency
- Like @alflennik pointed out we would be maintaining this
- Fragment support
- Internal dependency
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.
I'm really thankful you opened this up to early review Z, because this is going to take all my brainpower to review, haha, and definitely multiple passes. It's really amazing how you were able to turn this file upside down and right side up again. I think this file has been beyond mortal reach for a while, but with these changes I definitely feel like that is about to change.
86420f3
to
a2b93a2
Compare
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.
Outside of nitpicks and what's been spoken about before, I'm finding it difficult to add much from a technical standpoint. This is without a doubt, impressive! I also believe it has been tested well enough to some degree to be convinced it does exactly what it has been proposed to do.
However, my general concern is around the maintainability of it, to also echo @alflennik, should any additional work ever be required. I know things are still changing but maybe some additional descriptions should start getting added in with the newly added JSDoc
tags (which is impressive in and of itself as well) to describe further what these inputs are, especially the map
, object
and any
properties. This should be helpful to others down the line in helping to alleviate this concern.
In a previous meeting we had as well, you provided very useful descriptions on the overall purpose of each file which I believe helped my understanding immensely going in. So having a similar write-up could be helpful to capture your initial thoughts for others as well I believe.
* @typedef {EnumValues<typeof AdditionalAssertionResultMap>} AdditionalAssertionResult | ||
*/ | ||
|
||
export const AdditionalAssertionResultMap = createEnumMap({ |
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.
nit: I certainly agree with the separation of concerns for the Map
s here but it does feel like a few of these could be packaged into 1, especially the ...AssertionMap
s because they are related and it could limit the amount of documentation/'looking for property in wrong map' errors one could possibly encounter? Just a thought.
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.
Hmm. Yeah at this point I just wanted to mirror the values that get exported with ones in the internal state that follow a common idiom.
|
||
export const AdditionalAssertionResultMap = createEnumMap({ | ||
...CommonResultMap, | ||
FAIL_SUPPORT: "failSupport", |
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.
nit: I noticed the values of these generated Map
s all seem to differ between camelCase, natural language and capitalized. I figure it has to do with where their respective values will be used. Perhaps their appropriate usage could be described in their JSDoc tags?
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.
Yeah. The natural language, spaced and title cased, values and the all capitalized values are values as they already appear in the json object built and submitted from the test result from input. I'll add something like that to the doc tags.
- add an Application to the harness that maintains an internal state, produces a non-dom document with all the page's structure and text and representation of the internal state and functions to change state and produce a new accompanying document - add a reduced in-repo virtual dom renderer - render the non-dom document with the virtual dom renderer
62f38c6
to
1f54b16
Compare
The harness module is given a test json of commands to perform and assertions to record the results of. This change replaces the manual DOM rendering and manual updates of the test instructions and result form with an object representing the test results, from which a second object, the instruction document, is created representing the structure, text, and user event handlers to update the first object. This separates the application state of the instructions and result entry from the DOM so that it can be reused outside the harness module.
This change further uses the added instruction document type and produces the existing DOM with a small virtual dom module. From the perspective of someone running the commands and entering the results into the form, there should be no change in the page’s behavior.
Progress
This change is almost complete. Briefly, I think to complete it, the result state needs to be turned into the summary object for submission. Below is an overview of the steps implemented.
behavior
module scope variable