Skip to content

Commit

Permalink
web: streamline CaptchaStage
Browse files Browse the repository at this point in the history
# What

This commit:

1. Replaces the mass of `if () { if() { if() } }` with two state tables:
  - One for `render()`
  - One for `renderBody()`

2. Breaks each Captcha out into "interactive" and "executive" versions
3. Creates a handler table for each Captcha type
4. Replaces the `.forEach` expression with a `for` loop.
5. Move `updated` to the end of the class.
6. Make captchDocument and captchaFrame constructed-on-demand with a cache.
7. Remove a lot of `@state` handlers
8. Give IframeMessageEvent its own type.
9. Removed `this.scriptElement`
10. Replaced `window.removeEventListener` with an `AbortController()`
# Why

1. **Replacing `if` trees with a state table.** The logic of the original was really hard to follow.
   With the state table, we can clearly see now that for the `render()` function, we care about the
   Boolean flags `[embedded, challenged, interactive]` and have appropriate effects for each. With
   `renderBody()`, we can see that we care about the Boolean flags `[hasError, challenged]`, and can
   see the appropriate effects for each one.

2. (and 3.) **Breaking each Captcha clause into separate handlers.** Again, the logic was convoluted,
   when what we really care about is "Does this captcha have a corresponding handler attached to
   `window`, and, if so, should we run the interactive or executive version of it?" By putting all
   of that into a table of `[name, challenge, execute]`, we can clearly see what's being handled
   when.

4. **Replacing `foreach()` with `for()`**: [You cannot use a `forEach()` with async
   expressions](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#:~:text=does%20not%20wait%20for%20promises).
   If you need asynchronous behavior in an ordered loop, `for()` is the safest way to handle it; if
   you need asynchronous behavior from multiple promises, `Promise.allSettled(handlers.map())` is
   the way to go.

   I tried to tell if this function *meant* to run every handler it found simultaneously, or if it
   meant to test them in order; I went with the second option, breaking and exiting the loop once a
   handler had run successfully.

5. **Reordered the code a bit**. We're trying to evolve a pattern in our source code: styles,
   properties, states, internal variables, constructor, getters & setters that are not `@property()`
   or `@state()`, DOM-related lifecycle handlers, event handlers, pre-render lifecycle handlers,
   renderers, and post-render lifecycle handlers. Helper methods (including subrenderers) go above
   the method(s) they help.

6. **Constructing Elements on demand with a cache**. It is not guaranteed that we will actually need
   either of those. Constructing them on demand with a cache is both performant and cleaner.
   Likewise, I removed these from the Lit object's `state()` table, since they're constructed once
   and never change over the lifetime of an instance of `ak-stage-captcha`.

9. **Remove `this.scriptElement`**: It was never referenced outside the one function where it was used.

10. **Remove `removeEventListener()`**: `AbortController()` is a bit more verbose for small event
    handler collections, but it's considered much more reliable and much cleaner.
  • Loading branch information
kensternberg-authentik committed Dec 4, 2024
1 parent fe9e452 commit 3344476
Showing 1 changed file with 204 additions and 167 deletions.
Loading

0 comments on commit 3344476

Please sign in to comment.