web/flows: resize captcha iframes (cherry-pick #12260)#12304
Merged
BeryJu merged 1 commit intoversion-2024.10from Dec 9, 2024
Merged
web/flows: resize captcha iframes (cherry-pick #12260)#12304BeryJu merged 1 commit intoversion-2024.10from
BeryJu merged 1 commit intoversion-2024.10from
Conversation
* web: Add InvalidationFlow to Radius Provider dialogues
## What
- Bugfix: adds the InvalidationFlow to the Radius Provider dialogues
- Repairs: `{"invalidation_flow":["This field is required."]}` message, which was *not* propagated
to the Notification.
- Nitpick: Pretties `?foo=${true}` expressions: `s/\?([^=]+)=\$\{true\}/\1/`
## Note
Yes, I know I'm going to have to do more magic when we harmonize the forms, and no, I didn't add the
Property Mappings to the wizard, and yes, I know I'm going to have pain with the *new* version of
the wizard. But this is a serious bug; you can't make Radius servers with *either* of the current
dialogues at the moment.
* web: streamline CaptchaStage
# 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.
* Didn't save the extracted ListenerController.
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## version-2024.10 #12304 +/- ##
===================================================
- Coverage 92.67% 92.60% -0.07%
===================================================
Files 761 761
Lines 37953 37953
===================================================
- Hits 35173 35147 -26
- Misses 2780 2806 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This file contains hidden or 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
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.
Cherry-picked web/flows: resize captcha iframes (#12260)
What
{"invalidation_flow":["This field is required."]}message, which was not propagatedto the Notification.
?foo=${true}expressions:s/\?([^=]+)=\$\{true\}/\1/Note
Yes, I know I'm going to have to do more magic when we harmonize the forms, and no, I didn't add the
Property Mappings to the wizard, and yes, I know I'm going to have pain with the new version of
the wizard. But this is a serious bug; you can't make Radius servers with either of the current
dialogues at the moment.
What
This commit:
if () { if() { if() } }with two state tables:render()renderBody().forEachexpression with aforloop.updatedto the end of the class.@statehandlersthis.scriptElementwindow.removeEventListenerwith anAbortController()Why
Replacing
iftrees 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 theBoolean flags
[embedded, challenged, interactive]and have appropriate effects for each. WithrenderBody(), we can see that we care about the Boolean flags[hasError, challenged], and cansee the appropriate effects for each one.
(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 allof that into a table of
[name, challenge, execute], we can clearly see what's being handledwhen.
Replacing
foreach()withfor(): You cannot use aforEach()with asyncexpressions.
If you need asynchronous behavior in an ordered loop,
for()is the safest way to handle it; ifyou need asynchronous behavior from multiple promises,
Promise.allSettled(handlers.map())isthe 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.
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.
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 onceand never change over the lifetime of an instance of
ak-stage-captcha.Remove
this.scriptElement: It was never referenced outside the one function where it was used.Remove
removeEventListener():AbortController()is a bit more verbose for small eventhandler collections, but it's considered much more reliable and much cleaner.