Skip to content

Strongly typed custom events #808

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

Closed
ashubham opened this issue Sep 15, 2019 · 15 comments
Closed

Strongly typed custom events #808

ashubham opened this issue Sep 15, 2019 · 15 comments

Comments

@ashubham
Copy link

ashubham commented Sep 15, 2019

IMO, If properties are inputs to a Custom element, Custom Events are outputs. Currently properties are first class citizens, they can be annotated via decorators and type checked.

CustomEvents are defined like this:

// my-custom-element.ts

let event = new CustomEvent('my-event', {
      detail: {
        message: 'Something important happened'
      }
    });
this.dispath(event);

You listen on the parent like this

<my-custom-element @my-event=${this.handleMyEvent}></my-custom-element>

When listening for these events on the parent(s), the only source of API knowledge is documentation if it all exists else you are left to reading internal implementation. The types on detail key are not enforced. It will be great if lit-element could implement maybe decorators to annotate custom events emitted by an Element.

@LarsDenBakker
Copy link
Contributor

CustomEvent uses generics, so you can type the detail property with CustomEvent<YourTypeHere>.

You're not limited to using CustomEvent, you can and should create your own events if you're using types:

class MyEvent extends Event {
  ...
}

@chase-moskal
Copy link

@ashubham — i use this pattern regularly to attain strongly typed events

import {AuthContext} from "../interfaces.js"

export class UserLoginEvent extends CustomEvent<AuthContext> {
  static eventName = "user-login"

  constructor(detail: AuthContext) {
    super(UserLoginEvent.eventName, {
      detail,
      bubbles: true,
      composed: true
    })
  }
}

@ashubham
Copy link
Author

ashubham commented Sep 16, 2019

How do you annotate what Events are fired from a Component ? We can annotate properties, I created a decorator for my own use case something like this:

export class MyComponent extends LitElement {
  @property({type: String}) prop: string = '';
  @event<string> myEventName!: (string) => void;

  render() {
    return html`<div @click=${this.onClick}></div>`
  }

  private onClick(e) {
    this.myEventName('my string');
  }

And the implementation of the decorator is something like:

export function event<T>() {
  return (component: LitElement, name) => {
    component[name] = (detail: T) => {
      this.dispatchEvent(new CustomEvent(name, {
        detail, bubbles: true, composed: true
      }));
    }
  }
}

this approach has a couple of problems:

  1. Code completion/checking on the consumer side of the component
    <my-component @myEventName=${this.onEvent}></my-component>. Does myEventName exists ? Maybe we can workaround this by using @fires jsdoc comment and using vscode lit plugin.

  2. What is the shape of the event triggered ? How do we export the types of the detail ie. the generic T to the consumer of my-component.

@chase-moskal
Copy link

chase-moskal commented Sep 16, 2019

i think you're onto a cool idea! this could be an ergonomic improvement over calling this.dispatchEvent directly

i would really like it if we could whip up an @event decorator, however there are so many ways we can go about this

[edit: redacted obsolete chatter now that my next comment is more complete]

👋 chase

@chase-moskal
Copy link

chase-moskal commented Sep 17, 2019

hello friend, i was inspired by your idea of an @event decorator, so i made this implementation

event-decorator.ts — this is the event decorator utility

export function dashify(camel: string) {
  return camel.replace(/([a-zA-Z])(?=[A-Z])/g, '$1-').toLowerCase()
}

export type EventDetails<T extends CustomEvent> =
  T extends CustomEvent<infer D> ? D : never

export type Dispatcher<E extends CustomEvent> = (
  options?: CustomEventInit<EventDetails<E>>
) => void

export function dashifyEventName(EventClass: Function) {
  const dashed = dashify(EventClass.name)
  const result = /^(.*)-event$/i.exec(dashed)
  return result ? result[1] : dashed
}

export function prepareEventDecorator(
  fallbackOptions: CustomEventInit = {}
) {
  return function event<E extends CustomEvent>(
    EventClass: new(...args: any[]) => E,
    name2?: string,
  ): PropertyDecorator {
    const name = name2 || dashifyEventName(EventClass)
    return (target, key) => {
      target[key] = <any>function(this, options = {}) {
        this.dispatchEvent(new EventClass(
          name,
          {...fallbackOptions, ...options}
        ))
      }
    }
  }
}

export const event = prepareEventDecorator()

events.ts — this is how i create my event classes

import {AuthContext, Profile} from "./interfaces.js"
import {
  Dispatcher,
  dashifyEventName,
  prepareEventDecorator,
} from "./event-decorator.js"

// here i export my own event decorator
// configured with my own defaults
// (otherwise the browser defaults to false and false)
export const event = prepareEventDecorator({
  bubbles: true,
  composed: true
})

// the event names are derived directly from the class names
// by converting camel case to dashes
// so "UserLoginEvent" becomes "user-login" automatically,
// this is good for maintenence so there is a single source of truth for the event names
// which can be easily refactored in an ide
export class UserLoginEvent extends CustomEvent<AuthContext> {}
export class UserLogoutEvent extends CustomEvent<{}> {}
export class ProfileLoadedEvent extends CustomEvent<Profile> {}

export {Dispatcher, dashifyEventName}

user-panel.ts

import {LitElement} from "lit-element"
import {
  event,
  Dispatcher,
  UserLoginEvent,
  UserLogoutEvent,
} from "./events.js"

export class UserPanel extends LitElement {
  @event(UserLoginEvent) dispatchUserLogin: Dispatcher<UserLoginEvent>
  @event(UserLogoutEvent) dispatchUserLogout: Dispatcher<UserLogoutEvent>

  private async _login() {
    const authContext = await this._doAuthStuff()
    this.dispatchUserLogin({detail: authContext})
     //                         ↑
     //             (detail object is correctly typed based on the event)
  }

  private async _logout() {
    await this._clearAuthStuff()
    this.dispatchUserLogout()
  }
}

it took me awhile to brain my way through the typings for the decorator to infer correctly, thankfully with the help of some fine folks on my twitch stream

  • declaring the property type as Dispatcher<CustomEvent<Detail>> allows the typings to flow through to the calling of the dispatcher function, thus that the detail type must be correct
  • you can also specify an event name while using the event decorator, if you don't want to use the auto-generated dashed name: @event(UserLoginEvent, "my-login-event") dispatchLogin: Dispatch<UserLoginEvent>
  • you can also specify other EventInit options for each dispatch call like this: this.dispatchUserLogin({detail: authContext, bubbles: true, composed: true})

i've implemented this in my open source auth software i'm working on: https://github.com/chase-moskal/authoritarian-client/blob/refactor/source/toolbox/event-decorator.ts

let me know what you think, cheers!

👋 chase

@chase-moskal
Copy link

chase-moskal commented Sep 17, 2019

a friend of mine just suggested a cool idea

we should also make a property decorator to whip up event handlers that get automatically attached and removed upon connectedCallback and disconnectedCallback

class MyComponent extends LitElement {
  @listener(["scroll", "resize", MySizingEvent]) handleResize = event => {
    console.log("blah")
  }
}

i believe it will require a class decorator to mixin the functionality to make it work

i'm interested to create a publish these ideas to a library soon, i'd love to hear if anybody has any alternative approaches

👋 chase

@chase-moskal
Copy link

chase-moskal commented Sep 17, 2019

i've just made an implementation in a new repository that i'm pretty stoked about

with only two decorators, @event and @listener, we can declare, dispatch, and listen for events on our web components

  • mega-robot.ts example component declares and dispatches events
    • using @bubblingEvent decorator
  • robot-vizualizer.ts example component listens for events
    • using @listener decorator
    • automatically adds/removes event listeners when component is connected/disconnected
    • replaces the listener property with an object containing:
      • this._handleRobotSelfAware.handler: actual handler
      • this._handleRobotSelfAware.remove(): function to remove handler manually
      • this._handleRobotSelfAware.add(): function to add handler manually

each decorator takes, as a second argument, an optional options object

i'm glad i was able to avoid needing to implement a class mixin for this, making it work with two decorators rather than three

once i come up with a better name than eventatarian, i'm going to publish this to npm

now it's time for breakfast, cheers!

👋 chase

@ashubham
Copy link
Author

@chase-moskal thanks for the effort. I think in the examples, we should define and export events from the component itself. The consumer can then import it from the component itself.

However, this does not address the requirement of being able to typecheck it on the consumer side.

render() {
  html`<mega-robot @robot-self-aware=${this.handleRobotSelfAware}></mega-robot>`;
}

handleRobotSelfAware(e: <What type to put here?>) {
 // ...
}

@chase-moskal
Copy link

chase-moskal commented Sep 18, 2019

@ashubham

unfortunately, with the way typescript works, we cannot export the event types directly from the component itself, however,

for your case, to gain the typings you want, you should define/use custom event classes, like this:

export interface RobotAwareness {
  level: number
}

export class RobotSelfAwareEvent extends CustomEvent<RobotAwareness> {}

then you can use the event types in your handlers, like this:

private _handleRobotSelfAware = (event: RobotSelfAwareEvent) => {
  console.log(event.level)
}

i went to great lengths to accomplish as much automatic type inference as possible, and i think the patterns here and in my example repo represent the best we can do on this front

i'm quite pleased with the results, and will begin implementing these patterns in my work tomorrow

sometime soon, i'll be publishing the decorator library to npm, and writing a readme to explain the best practices

let me know and i'd be happy to chat further about this with you on discord or on my twitch stream, cheers!

👋 chase

@chase-moskal
Copy link

i've renamed and published the package as "event-decorators"

@justinfagnani
Copy link
Contributor

I want to chime in, since this thread has been going on for a while...

Events are unlike properties in that they don't have a standard class-member representation. So there's nothing for TypeScript to be able to read statically from the element definition to determine what events are fired and what their types are.

However, JSDoc has the @fires tag and @event tag which can be used to tell documentation generators about the events. I think they need a little modernization to use them with DOM events however.

TypeScript also has the ElementEventMap which associates event names with types. This is unfortunately global, but it can help for type-checking with addEventListener(). You add an event type to it like you add a custom element type to HTMLElementTagNameMap:

interface ElementEventMap {
    'my-event': MyEvent;
}

Event decorators are interesting, but I personally don't see the need for creating event listener properties when we have the more general addEventListener and dispatchEvent methods. Given that we want to 1) keep LitElement as small as possible, and 2) support TypeScript transforms that completely compile out decorators (transform them to plain class members), I wouldn't want to add more decorators to the core lit-element package.

Great conversation however, and a really good area for various community opinions and add-ons. I'll close this for now.

@chase-moskal
Copy link

I personally don't see the need for creating event listener properties when we have the more general addEventListener and dispatchEvent methods

i previously found myself doing a lot of boilerplate addEventListener and removeEventListener in connectedCallback and disconnectedCallback, and the decorators can help cut down on the noise

there isn't much benefit over perceived readability if it floats your boat ⛵

cheers!

👋 chase

@justinfagnani
Copy link
Contributor

i previously found myself doing a lot of boilerplate addEventListener and removeEventListener in connectedCallback and disconnectedCallback

Why the removeEventListener in disconnectedCallback? I find adding host listeners in the constructor to be just fine:

class MyElement extends HTMLElement {
  constructor() {
    super();
    this.addEventListener('click', this._onClick);
  }

  _onClick = (e) => {
    console.log('click', this);
  };
}

@chase-moskal
Copy link

chase-moskal commented Sep 19, 2019

Why the removeEventListener in disconnectedCallback? I find adding host listeners in the constructor to be just fine

that's a great question! i suppose for host listeners, that's perfectly reasonable, as i imagine when elements are disconnected, all attached listeners become irrelevant/forgotten

however removing event listeners in disconnectedCallback is important when the event target is anything else, like window — or at least this is what i assume, i don't suppose the browser knows to remove the correct window event listeners when a web component is disconnected? (that would be some unexpected funky magic in my estimation)

at the moment, my use-case happens to involve several components that are interacting in a decoupled way via events that bubble up to the window

i also happen to like what the decorators do for the readability of the components — visually the @listener decorators are very clear, and leave only one relevant spot for each listener (rather than two like with _onClick versus addEventListener) — the @event decorators are also a nice self-documenting visual aid

in my event-decorators library, for the case of lit-element, i wonder if it makes sense to add the listeners in firstUpdated instead of connectedCallback, such that the event target could be a property that was assigned via <x-lol .eventTarget=${myTarget}>.. perhaps i could add a string option for which callback gets hitched.. and add @litListener for that purpose.. hmm, food for thought!

👋 chase

@difosfor
Copy link

Interesting!

You don't have to modify the global ElementEventMap to add support for your custom event types. I'm currently using this approach: https://gist.github.com/difosfor/ceeb01d03a8db7dc68d5cd4167d60637

I like the idea of adding support for strongly typed custom events to LitElement, particularly using decorators. Ideally I'd also like other tooling like web-component-analyzer and VS Code lit-plugin to use the type information and descriptions instead of requiring me to additionally document the events using @fires tags.

The search for a fully functional typed custom element approach continues.. 😉
Please ping me if you have any further suggestions!

kensternberg-authentik added a commit to goauthentik/authentik that referenced this issue Sep 29, 2023
This commit refactors the various components of the Wizard and ApplicationWizard, creating a much
more maintainable and satisfying Wizard experience for both developers (i.e, *me* and *Jens* so
far), and for the customer.

The Wizard base has been refactored into three components:

**AkWizardController**

The `AkWizardController` provides the event listenters for the wizard; it hooks them up, recevies the
events, and forwards them to the wizard.  It unwraps the event objects and forwards the relevant
messages contained in the events.  It knows of three event categories:

- Navigation requests (move to a different step)
- Update requests (the current step has updated the business content)
- Close requests (close or cancel the wizard).

**ak-wizard-frame**

The `ak-wizard-frame` is the ModalButton interface.  It provides the Header, Breadcrumbs (nee`
"navigation block"), Buttons, and a DIV into which the main content is rendered.

**AkWizard**

`AkWizard` is an *incomplete* implementation of the wizard. It's meant to be inherited by a child
class, which will implement the rest. It extends `AKElement`. It provides the basic content needed,
such as steps, currentStep (as an index), an accessor for the step itself, an accessor for the
frame, and the interface to the `AkWizardController`.

**ApplicationWizard**

The `ApplicationWizard` itself has been refactored to accommodate these changes. It inherits from
`AkWizard` and provides the business logic for what to do when a form updates, some custom logic for
preventing moving through the wizard when the forms are incomplete, and a persistence layer for
filling out different providers in the same session. It's simplified a *lot*.

The types specified for `AkWizard` are pretty nifty, I think. I could wish the types being passed
via the custom events were more robust, but [strongly typed custom
events](lit/lit-element#808) turn out to be quite the pain in the, er,
neck. As it is, the `precommit` pass did very good at preventing the worst disasters.

The steps themselves were re-written as objects so that they could take advantage of their `valid`
and `disabled` states and provide more meaningful buttons and labels. I think it's a solid
compromise, and it moved a lot of display logic out of the core `handleUpdate()` business method.

The tests, such as they are, are passing.
kensternberg-authentik added a commit to goauthentik/authentik that referenced this issue Oct 18, 2023
* A lot of comments about forms.

* Adding comments to the wizard.

* Broke out the text input into a single renderer.  Still works as required.

* web: Legibility in the ApplicationForm.

This is a pretty good result.  By using the LightDOM setting, this
provides the existing Authentik form manager with access to the
ak-form-horizontal-element components without having to do any
cross-border magic.  It's not ideal, and it shows up just how badly
we've got patternfly splattered everywhere, but the actual results
are remarkable.  The patterns for text, switch, radio, textarea,
file, and even select are smaller and easier here.

I'm still noodling on what an unspread search-select element would
look like.  It's just dependency injection, so it ought to be as
straightforward as that.

* web: Marking down the start of the 'components' library.

* web: Baby steps

I become frustrated with my inability to make any progress on this project, so I decided to reach
for a tool that I consider highly reliable but also incredibly time-consuming and boring: test
driven development.

In this case, I wrote a story about how I wanted to see the first page rendered: just put the HTML
tag, completely unadorned, that will handle the first page of the wizard. Then, add an event handler
that will send the updated content to some parent object, since what we really want is to
orchestrate the state of the user's input with a centralized location. Then, rather than fiddling
with the attributes and properties of the various pages, I wanted them to be able to "look up" the
values they want, much as we'd expect a standalone form to be able to pull its values from the
server, so I added a context object that receives the update event and incorporates the new
knowledge about the state of the process into itself.

The result is surprisingly satisfying: the first page renders cleanly, displays the content that we
want, and as we fiddle with, we can *watch in real time* as the results of the context are updated
and retransmitted to all receiving objects. And the sending object gets the results so it
re-renders, but it ends up looking the same as it was before the render.

* Now, it's starting to look like a complete package. The LDAP method is working, but there is a bug:
the radio is sending the wrong value !?!?!?. Track that down, dammit. The search wrappers now resend
their events as standard `input` events, and that actually seems to work well; the browser is
decorating it with the right target, with the right `name` attribute, and since we have good
definitions of the `value` as a string (the real value of any search object is its UUID4), that
works quite well. Added search wrappers for CoreGroup and CryptoCertificate (CertificateKeyPairs),
and the latter has flags for "use the first one if it's the only one" and "allow the display of
keyless certificates."

Not sure why `state()` is blocking the transmission of typing information from the typed element
to the context handler, but it's a bug in the typechecker, and it's not a problem so far.

* Now, it's starting to look like a complete package. The LDAP method is working, but there is a bug:
the radio is sending the wrong value !?!?!?. Track that down, dammit. The search wrappers now resend
their events as standard `input` events, and that actually seems to work well; the browser is
decorating it with the right target, with the right `name` attribute, and since we have good
definitions of the `value` as a string (the real value of any search object is its UUID4), that
works quite well. Added search wrappers for CoreGroup and CryptoCertificate (CertificateKeyPairs),
and the latter has flags for "use the first one if it's the only one" and "allow the display of
keyless certificates."

Not sure why `state()` is blocking the transmission of typing information from the typed element
to the context handler, but it's a bug in the typechecker, and it's not a problem so far.

* web: tracked down that weirld bug with the radio.

Because radio inputs are actually multiples, the events handling for
radio is... wonky.  If we want our `<ak-radio>` component to be a
unitary event dispatcher, saying "This is the element selected," we
needed to do more than what was currently being handled.

I've intercepted the events that we care about and have placed
them into a controller that dictates both the setting and the
re-render of the component.  This makes it "controlled" (to use the
Angular/React/Vue) language and depends on Lit's reactiveElement
lifecycle to work, rather than trust the browser, but the browser's
experience with respect to the `<input type=radio` is pretty bad:
both input elements fire events, one for "losing selection" and
one for "gaining selection".  That can be very confusing to handle,
so we funnel them down in our aggregate radio element to a single
event, "selection changed".

As a quality-of-life measure, I've also set the label to be
unselectable; this means that a click on the label will trigger the
selection event, and a long click will not disable selection or
confuse the selection event generator.

* web: now passing the precommit phase

* web: a HACK for Storybook to inject the "use light theme" flag into the body.

This isn't really a very good hack; what it does is say that every story is
responsible for hacking its theme into the parent.  This is very annoying, but
it does mean that we can at least show our components in the best light.

* web: ak-application-wizard-authentication-by-oauth, and many fixes!

1. Fixed `eventEmitter` so that if the detail object is a scalar, it will not attempt to "objectify"
   it.  This was causing a bug where retrofitting the eventEmitter to some older components resulted
   in a detail of "some" being translated into ['s', 'o', 'm', 'e'].  Not what is wanted.
2. Removed the "transitional form" from the existing components; they had a two-step where the web
   component class was just a wrapper around an independent rendering function.  While this worked,
   it was only to make the case that they *were* independent rendering objects and could be
   supported with the right web component framework.  We're halfway there now; the last step will be
   to transform the horizontal-element and various input CSS into componentized CSS, the way
   Patternfly-Elements is currently doing.
3. Fixed the `help` field so that it could take a string or a TemplateResult, and if the latter,
   don't bother wrapping it in the helper text functionality; just let it be its own thing.  This
   supports the multi-line help of redirectURI as well as the `ak-utils-time-delta` capability.
4. Transform Oauth2ProviderForm to use the new components, to the best of our ability.  Also used
   the `provider = this.wizard.provider` and `provider = this.instance` syntax to make the render
   function *completely portable*; it's the exact same text that is dropped into...
5. The complete `ak-application-wizard-authentication-by-oauth` component. They're so similar part
   of me wonders if I could push them both out to a common reference, or a collection of common
   references.  Both components use the PropertyMapping and Sources, and both use the same
   collection of searches (Crypto, Flow).
6. A Storybook for `ak-application-wizard-authentication-by-oauth`, showing the works working.
7. New mocks for `authorizationFlow`, `propertyMappings`, and `hasJWKs`.

This sequence has revealed a bug in the radio control.  (It's always the radio control.)  If the
default doesn't match the current setting, the radio control doesn't behave as expected; it won't
change when you fully expect that it should.  I'll investigate how to harmonize those tomorrow.

* web: Converted our toggle groups to a more streamlined implementation.

* web: one more toggle group.

* initial api and schema

Signed-off-by: Jens Langhammer <[email protected]>

* separate blueprint importer from yaml parsing

Signed-off-by: Jens Langhammer <[email protected]>

* cleanup

Signed-off-by: Jens Langhammer <[email protected]>

* web: Replace ad-hoc toggle control with ak-toggle-group

This commit replaces various ad-hoc implementations of the Patternfly Toggle Group HTML with a web
component that encapsulates all of the needed behavior and exposes a single API with a single event
handler, return the value of the option clicked.

The results are: Lots of visual clutter is eliminated.  A single link of:

```
<div class="pf-c-toggle-group__item">
  <button
      class="pf-c-toggle-group__button ${this.mode === ProxyMode.Proxy
          ? "pf-m-selected"
          : ""}"
      type="button"
      @click=${() => {
          this.mode = ProxyMode.Proxy;
      }}>
      <span class="pf-c-toggle-group__text">${msg("Proxy")}</span>
  </button>
</div>
<div class="pf-c-divider pf-m-vertical" role="separator"></div>
```

Now looks like:

```
<option value=${ProxyMode.Proxy}>${msg("Proxy")}</option>
```

This also means that the three pages that used the Patternfly Toggle Group could eliminate all of
their Patternfly PFToggleGroup needs, as well as the `justify-content: center` extension, which also
eliminated the `css` import.

The savings aren't as spectacular as I'd hoped: removed 178 lines, but added 123; total savings 55
lines of code.  I still count this a win: we need never write another toggle component again, and
any bugs, extensions or features we may want to add can be centralized or forked without risking the
whole edifice.

* web: minor code formatting issue.

* add new "must_created" state to blueprints to prevent overwriting objects

Signed-off-by: Jens Langhammer <[email protected]>

* web: adding a storybook for the ak-toggle-group component

* Bugs found by CI/CD.

* web: Replace ad-hoc search for CryptoCertificateKeyPairs with ak-crypto-certeficate-search

This commit replaces various ad-hoc implementations of `search-select` for CryptoCertificateKeyPairs
with a web component that encapsulates all of the needed behavior and exposes a single API.

The results are: Lots of visual clutter is eliminated.  A single search of:

```HTML
<ak-search-select
    .fetchObjects=${async (query?: string): Promise<CertificateKeyPair[]> => {
        const args: CryptoCertificatekeypairsListRequest = {
            ordering: "name",
            hasKey: true,
            includeDetails: false,
        };
        if (query !== undefined) {
            args.search = query;
        }
        const certificates = await new CryptoApi(
            DEFAULT_CONFIG,
        ).cryptoCertificatekeypairsList(args);
        return certificates.results;
    }}
    .renderElement=${(item: CertificateKeyPair): string => {
        return item.name;
    }}
    .value=${(item: CertificateKeyPair | undefined): string | undefined => {
        return item?.pk;
    }}
    .selected=${(item: CertificateKeyPair): boolean => {
        return this.instance?.tlsVerification === item.pk;
    }}
    ?blankable=${true}
>
</ak-search-select>
```

Now looks like:

```HTML
<ak-crypto-certificate-search certificate=${this.instance?.tlsVerification}>
</ak-crypto-certificate-search>
```

There are three searches that do not require there to be a valid key with the certificate; these are
supported with the boolean property `nokey`; likewise, there is one search (in SAMLProviderForm)
that states that if there is no current certificate in the SAMLProvider and only one certificate can
be found in the Authentik database, use that one; this is supported with the boolean property
`singleton`.

These changes replace 382 lines of object-oriented invocations with 36 lines of declarative
configuration, and 98 lines for the class.  Overall, the code for "find a crypto certificate" has
been reduced by 46%.

Suggestions for a better word than `singleton` are welcome!

* web: display tests for CryptoCertificateKeypair search

This adds a Storybook for the CryptoCertificateKeypair search, including
a mock fetch of the data.  In the course of running the tests, we discovered
that including the SearchSelect _class_ won't include the customElement declaration
unless you include the whole file!  Other bugs found: including the CSS from
Storybook is different from that of LitElement native, so much so that the
adapter needed to be included.  FlowSearch had a similar bug.  The problem
only manifests when building via Webpack (which Storybook uses) and not
Rollup, but we should support both in distribution.

* Fixed behavioral problem with the radio; the `if` there was
preventing the radio from reflecting the default correctly.
The observed behavior was that the radio wouldn't "activate"
until the item selected during the render pass was clicked on
first.

* Proxy Provider done.

* web: Tactical change.  Put all the variants on the second page; it's
a longer list, but it's also easier to manage than all those
required sub-options.

* Rounding out the catalog.

* web: SAML Manual Configuration

Added a 'design document' that just kinda describes what I'm trying
to do, in case I don't get this done by Friday Aug 11, 2023.

I had two tables doing the same thing, so I merged them and then
wrote a few map/filters to specialize them for those two use cases.

Along the way I had to fiddle with the ESLint settings so that
underscore-prefixed unused variables would be ignored.

I cleaned up the visual appeal of the forms in the LDAP application.

I was copy/pasting the "handleProviderEvent" function, so I pulled
it out into ApplicationWizardProviderPageBase.  Not so much a matter
of abstraction as just disliking that kind of duplication; it served
no purpose.

* Added SAML Story to Storybook.

* Web: This is coming together amazingly well.  Like, almost too well.

* web: 80% of the way there

This commit includes the first three pages of the wizard, the
completion of the wizard framework with evented handling, and control
over progression.

Some shortcomings of this design have become evident: it isn't
possible to communicate between the steps' wrappers, as they are
POJOs without access to the context.  An imperative decision-making
process has to be inserted in the orchestration layer,
which is kinda annoying.

But it looks good and it behaves correctly, to the extent that I've
given it behavior.  It's an excellent foundation.

* Linting.

* web: application wizard

Found where the hook for form validity should go.  Excellent!  Now I just need to incorporate
that basic validation into the business logic and we're good to go.

* Turns out that was one layer too many; the topmost component was fine for
maintaining the context.

* It looks like my brilliant strategy has hit a snag.

The idea is simple.  Let's start with this picture:

```
<application-wizard .steps=${[... a collection of step objects ...]}>
  <wizard-main .steps=${(steps from above)}>
    <application-current-panel>
      <current-form>
```

- ApplicationWizard has a Context for the ApplicationProviderPair (or whatever it's going to be).
  This context does not know about the steps; it just knows about: the "application" object, the
  "provider" object, and a discriminator to know *which* provider the user has selected.
- ApplicationWizard has Steps that, among other things, provides Panels for:
  - Application
  - Pick Provider
  - Configure Provider
  - Submit ApplicationProviderPair to the back-end
- The WizardFrame renders the CurrentPanel for the CurrentStep

The CurrentPanel gets its data from the ApplicationWizard in the form of a Context. It then sends
messages (events) to ApplicationWizard about the contents of each field as the user is filling out
the form, so that the ApplicationWizard can record those in the ApplicationProviderPair for later
submission.

When a CurrentForm is valid, the ApplicationWizard updates the Steps object to show that the "Next
button" on the Wizard is now available.

In this way, the user can progress through the system.  When they get to the last page, we can
provide in the ApplicationWizard with the means to submit the form and/or send the user back to
the page with the validation failure.

Problem: The context is being updated in real-time, which is triggering re-renders of the form. This
leads to focus problems as the fields that are not yet valid are triggering "focus grab" behavior.
This is a classic problem with "controlled" inputs. What we really want is for the CurrentPanel to
not re-render at all, but to behave like a normal, uncontrolled form, and let the browser do most of
the work.  We still want the [Next] button to enable when the form is valid enough to permit that.

---

Other details: I've ripped out a lot of Jen's work, which is probably a mistake.  It's still
preserved elsewhere.  I've also cleaned up the various wizardly things to try and look organized.
It *looks* like it should work, it just... doesn't.  Not yet.

* Late addition: I had an inspiration about how to reduce the way
reactivity broke focus by, basically, removing the reactivity and
managing the first-time-through lifecycle to prevent the update
from causing refocus.  It works well!  Now I just need to test it.

* This application fixes the bug with respect to the wizard-level context being updated incorrectly.

Understandings:

- To use uncontrolled inputs, which I prefer, the context object should not be a state or property
  at the level of consumers; it should not automatically re-render with every keystroke, i.e. "The
  React Way."  We're using Web Components, [client-side
  validation](https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation) exists on the
  platform already, and live-validation is problematic for any number of reasons.
- The trade-off is that it is now necessary to re-render the target page of the wizard de-novo, but
  that's not really as big a deal as it sounds. Lit is ready to do that... and then nothing else
  until we request a change-of-page. Excellent.
- The top level context *must* be a state, but it's better if it's a state never actually used by
  the top-level context container. The debate about whether or not to make that container a dumb one
  (`<slot></slot>`) or to merge it with the top-level object continues; here, I've merged it with
  the top-level wizard object, but that object does not refer to the state variable being managed in
  its render pass, so changes to it do not cause a re-render of the whole wizard. The purpose of the
  top-level page is to manage the *steps*, not the *content of any step*. A step may change
  dynamically based on the content of a step, but that's the same thing as *which step*. Lesson:
  always know what your state is *about*.
- Deep merging is a complex subject, but here it's appropriate to our needs.

* web: Application Wizard

This commit combines a working (but very unpolished) version of the Application Wizard with Jen's
code for the CoreTransactionApplicationRequest, resulting in a successful round trip.

It fixes a number of bugs with the way ContextProducer decorators were being processed, such that
they just weren't working with our current configuration (although they did work fine in Storybook);
consumers didn't need to be fixed.

It also *removes* the steps-aware context from the Wizard.

That *may* be a mistake.  To re-iterate, the `WizardFrame` provides the chrome for a Wizard: the
button bar div, the breadcrumbs div, the header div, and it takes the steps object as its source of
truth for all of the content.  The `WizardContent` part of the application has two parts: The
`WizardMain`, which wraps the frame and supplies the context for all the `WizardPanels`, and the
`WizardPanels` themselves, which are dependent on a context from `WizardMain` for the data that
populates each panel. YAGNI right now that the panels need to know anything about the steps, and the
`WizardMain` can just pass a fresh `.steps` object to the `WizardFrame` when they need updating.
Using props drilling may make more sense here.

It certainy does *not* make sense for the panels.  They need to be renderable on-demand, and they
need to make sense of what they're rendering on-demand, so the function is

```
(panel code) => (context) => (rendered panel)
```

(Yes, that's curried notation. Deal.)

* This commit includes the first WDIO test for the ApplicationWizard.  It doesn't do much right now, but
it does log in and navigate to the wizard successfully.

* web: completed test for single application, provided new programming language to make it easier to write tests.

* Almost there.

Missing: The validation is currently not working as expected, and I cannot get the backend
to give me meaningful data helping us "go back" to the field that wasn't valid.  I really
don't want to put all the meaningful validation on the front-end; that's the road to -
perdition, the back-end must be usable by people less assiduous than we are.

Also: Need to make the button bar work better; maybe each panel can provide a custom button
bar if one is needed?

* web: Test harness

We have an end-to-end test harness that includes a trivially correct DSL for "This is what a user would do, do this":

```
const deleteProvider = (theSlug) => ([
    ["button", '>>>ak-sidebar-item a[href="#/core/providers"]'],
    ["deletebox", `>>>a[href="#/core/applications/${theSlug}"]`],
    ["button", '>>>ak-forms-delete-bulk button[slot="trigger"]'],
    ["button", '>>>ak-forms-delete-bulk div[role="dialog"] ak-spinner-button'],
]);
```

It's now possible to target individual sequences of events this way.  With a little creativity, we could have standalone functions that take parameters for our calls and just do them, without too much struggle.

* web: Revised navigation

After working with the navigation for awhile, I realized that it's a poor map; what I really wanted was
a controller/view pair, where events flow up to the controller and then messages on "what to draw" flow
down to the view.  It work quite well, and the wizard frame is smaller and smarter for it.

I've also moved the WDIO-driven tests into the 'tests' folder, because it (a) makes more sense to put
them there, and (b) it prevents any confusion about who's in charge of node_modules.

* web: Simplify, simplify, simplify

Sort-of.

This commit changes the way the "wizard step coordinator" layer works, giving the
wizard writer much more power over button bar.  It still assumes there are only
three actions the wizard frame wants to commit: next, back, and close.  This empowers
the steps themselves to re-arrange their buttons and describe the rules through which
transitions occur.

* web: resetting the form is not working yet...

I vehemently dislike the object-oriented "reset" command; every wizard should start with
an absolutely fresh copy of the data upon entry.  Refactoring the wizard to re-build its
content from the inside is the correct way to go, but I don't have a good mental image
of how to make the ModalButton and the component it invokes interact cleanly, which
frustrates the hell out of me.

* web: reset

As I said, I greatly dislike having to be dependent upon "resets"; I prefer my
data to be de novo going into a "new" transaction.  That said, we work with
what we've got; I've created an event generated by the wizard that says the
modal just closed; anything wrapping and implementing the wizard can then
capture that event and reset the data.  I've also added a pair of functions
that create the two states (what step, what form data) anew, so that resetting
is as trivial as initializing (and is exactly the same, code-wise).

* web: Without error handling, this is complete, but I still need @BeryJu (Jens)
for help with the SAML Upload (it doesn't appear to be correctly handled?) and
the error handling.

* web: revise tests for wizard

This commit replaces the previous WDIO instance with a more formal and straightforward process using
the [pageobjects](https://martinfowler.com/bliki/PageObject.html).  In this form, every major
component has its own test suite, and a test is a sequence of exercises of those components.

A test then becomes something as straightforward as:

```
        await LoginPage.open();
        await LoginPage.login("[email protected]", "eat10bugs");

        expect(await UserLibraryPage.pageHeader).toHaveText("My Applications");
        await UserLibraryPage.goToAdmin();

        expect(await AdminOverviewPage.pageHeader).toHaveText("Welcome, ");
        await AdminOverviewPage.openApplicationsListPage();

        expect(await ApplicationsListPage.pageHeader).toHaveText("Applications");
        ApplicationsListPage.startCreateApplicationWizard();

        await ApplicationWizard.app.name.setValue(`Test application ${newId}`);
        await ApplicationWizard.nextButton.click();
        await (await ApplicationWizard.getProviderType("ldapprovider")).click();
        await ApplicationWizard.nextButton.click();
        await ApplicationWizard.ldap.setBindFlow("default-authentication-flow");
        await ApplicationWizard.nextButton.click();
        await expect(await ApplicationWizard.commitMessage).toHaveText(
            "Your application has been saved"
        );
```

Whether or not there's another layer of DSL in there or not, this is a pretty nice idiom for
maintaining tests.

* web: updating with forms and fixes for eslint complaints.

* web/add webdriverIO testing layer

This commit adds WebdriverIO as an end-to-end solution to unit testing.  WebdriverIO can be run both
locally and remotely, supports strong integration with web components, and is generally robust for
use in pipelines.  I'll confess to working through a tutorial on how to do this for web components,
and this is just chapter 2 (I think there are 5 or so chapters...).

There's a makefile, with help!  If you just run `make` it tells you:

```
Specify a command. The choices are:

  help                 Show this help
  node_modules         Runs `npm install` to prepare this feature
  precommit            Run the precommit: spell check all comments, eslint with sonarJS, prettier-write
  test-good-login      Test that we can log into the server. Requires a running instance of the server.
  test-bad-login       Test that bad usernames and passwords create appropriate error messages
```

... because Makefiles are documentation, and documentation belongs in Makefiles.

I've chosen to go with a PageObject-oriented low-level DSL; what that means is that for each major
components (a page, a form, a wizard), there's a class that provides human-readable names for
human-interactable and human-viewable objects on the page.  The LoginPage object, for example, has
selectors for the username, password, submit button, and the failure alert; accessing those allows
us to test for items as expected., and to write a DSL for "a good login" that's as straightforward
as:

```
        await LoginPage.open();
        await LoginPage.login("[email protected]", "eat10bugs");
        await expect(UserLibraryPage.pageHeader).toHaveText("My applications");
```

There was a *lot* of messing around with the LoginPage to get the username and password into the
system.  For example, I had to do this with all the `waitForClickable` and `waitForEnable` because
we both keep the buttons inaccessible until the form has something and we "black out" the page (put
a darkening filter over it) while accessing the flow, meaning there was a race condition such that
the test would attempt to interact with the username or password field before it was accessible.
But this works now, which is very nice.

``` JavaScript
    get inputUsername() {
        return $('>>>input[name="uidField"]');
    }

    get btnSubmit() {
        return $('>>>button[type="submit"]');
    }

    async username(username: string) {
        await this.inputUsername.waitForClickable();
        await this.inputUsername.setValue(username);
        await this.btnSubmit.waitForEnabled();
        await this.btnSubmit.click();
    }
```

The bells & whistles of *Prettier*, *Eslint*, and *Codespell* have also been enabled. I do like my
guardrails.

* web/adding tests: added comments and cleaned up some administrative features.

* web/test: changed the name of one test to reflect it's 'good' status

* core/allow alternative postgres credentials

This commit allows the `dev-reset` command in the Makefile to pick up and use credentials from the
`.env` file if they are present, or fallback to the defaults provided if they are not. This is the
only place in the Makefile where the database credentials are used directly against postgresql
binaries. The syntax was tested with bash, zsh, and csh, and did not fail under those.

The `$${:-}` syntax is a combination of a Makefile idiom for "Pass a single `$` to the environment
where this command will be executed," and the shell expresion `${VARIABLE:-default}` means
"dereference the environment variable; if it is undefined, used the default value provided."

* Re-arrange sequence to avoid recursive make.

Nothing wrong with recursive make; it just wasn't essential
here.  `migrate` is just a build target, not a task.

* Cleanup according to the Usage:
  checkmake [options] <makefile>...
  checkmake -h | --help
  checkmake --version
  checkmake --list-rules Makefile linting tool.

* core: added 'help' to the Makefile

* get postgres config from authentik config loader

Signed-off-by: Jens Langhammer <[email protected]>

* don't set -x by default

Signed-off-by: Jens Langhammer <[email protected]>

* sort help

Signed-off-by: Jens Langhammer <[email protected]>

* update help strings

Signed-off-by: Jens Langhammer <[email protected]>

* web: test LDAP wizard sequence

* web: improve testing by adding test admin user via blueprint

* This commit continues the application wizard buildout.  In this commit are the following changes:

- Added SCIM to the list of available providers
- Fixed ForwardProxy so that its mode is set correctly.  (This is a special case in the committer;
  I'm unhappy with that.)
- Fixed the commit messages so that:
  - icons are set correctly (Success, Danger, Working)
  - icons are colored correctly according to state
  - commit message includes a `data-commit-state` field so tests can find it!
- Merged the application wizard tests into a single test pass
- Isolated common parts of the application wizard tests to reduce unnecessary repetition.  All
  application tests are the same until you reach the provider section anyway.
- Fixed the unit tests so they're finding the right error messages and are enabled to display them
  correctly.
- Moved the test Form handlers into their own folder so they're not cluttering up the Pages folder.

* web: add radius to application wizard

This commit continues the application wizard buildout.  In this commit are the following changes:

- Fixed a width-setting bug in the Makefile `make help` feature (i.e "automate that stuff!")
- Added Radius to the list of providers we can offer via the wizard
- Added `launchUrl` and `UI Settings` to features of the application page the wizard can find
- Changed 'SAML Manual Configuration' to just say "SAML Configuration"
- Modified `ak-form-group` to take and honor the `aria-label` property (which in turn makes it
  easier to target specific forms with unit testing)
- Reduced the log level for wdio to 'warn'; 'info' was super-spammy and not helpful.  It can be put
  back with `--logLevel info` from the command line.

* fix blueprints

Signed-off-by: Jens Langhammer <[email protected]>

* update package name

Signed-off-by: Jens Langhammer <[email protected]>

* add dependabot

Signed-off-by: Jens Langhammer <[email protected]>

* prettier run

Signed-off-by: Jens Langhammer <[email protected]>

* add basic CI

Signed-off-by: Jens Langhammer <[email protected]>

* remove hooks

Signed-off-by: Jens Langhammer <[email protected]>

* web: application wizard refactor & completion

This commit refactors the various components of the Wizard and ApplicationWizard, creating a much
more maintainable and satisfying Wizard experience for both developers (i.e, *me* and *Jens* so
far), and for the customer.

The Wizard base has been refactored into three components:

**AkWizardController**

The `AkWizardController` provides the event listenters for the wizard; it hooks them up, recevies the
events, and forwards them to the wizard.  It unwraps the event objects and forwards the relevant
messages contained in the events.  It knows of three event categories:

- Navigation requests (move to a different step)
- Update requests (the current step has updated the business content)
- Close requests (close or cancel the wizard).

**ak-wizard-frame**

The `ak-wizard-frame` is the ModalButton interface.  It provides the Header, Breadcrumbs (nee`
"navigation block"), Buttons, and a DIV into which the main content is rendered.

**AkWizard**

`AkWizard` is an *incomplete* implementation of the wizard. It's meant to be inherited by a child
class, which will implement the rest. It extends `AKElement`. It provides the basic content needed,
such as steps, currentStep (as an index), an accessor for the step itself, an accessor for the
frame, and the interface to the `AkWizardController`.

**ApplicationWizard**

The `ApplicationWizard` itself has been refactored to accommodate these changes. It inherits from
`AkWizard` and provides the business logic for what to do when a form updates, some custom logic for
preventing moving through the wizard when the forms are incomplete, and a persistence layer for
filling out different providers in the same session. It's simplified a *lot*.

The types specified for `AkWizard` are pretty nifty, I think. I could wish the types being passed
via the custom events were more robust, but [strongly typed custom
events](lit/lit-element#808) turn out to be quite the pain in the, er,
neck. As it is, the `precommit` pass did very good at preventing the worst disasters.

The steps themselves were re-written as objects so that they could take advantage of their `valid`
and `disabled` states and provide more meaningful buttons and labels. I think it's a solid
compromise, and it moved a lot of display logic out of the core `handleUpdate()` business method.

The tests, such as they are, are passing.

* Added comment describing new test.

* web: ensuring copy from `main` is canon

* web: fixes after merge

* web: laying the groundwork for future expansion

This commit is a hodge-podge of updates and changes to the web.  Functional changes:

- Makefile: Fixed a bug in the `help` section that prevented the WIDTH from being accurately
  calculated if `help` was included rather than in-lined.

- ESLint: Modified the "unused vars" rule so that variables starting with an underline are not
  considered by the rule.  This allows for elided variables in event handlers.  It's not a perfect
  solution-- a better one would be to use Typescript's function-specialization typing, but there are
  too many places where we elide or ignore some variables in a function's usage that switching over
  to specialization would be a huge lift.

- locale: It turns out, lit-locale does its own context management.  We don't need to have a context
  at all in this space, and that's one less listener we need to attach t othe DOM.

- ModalButton: A small thing, but using `nothing` instead of "html``" allows lit better control over
  rendering and reduces the number of actual renders of the page.

- FormGroup: Provided a means to modify the aria-label, rather than stick with the just the word
  "Details."  Specializing this field will both help users of screen readers in the future, and will
  allow test suites to find specific form groups now.

- RadioButton: provide a more consistent interface to the RadioButton.  First, we dispatch the
  events to the outside world, and we set the value locally so that the current `Form.ts` continues
  to behave as expected.  We also prevent the "button lost value" event from propagating; this
  presents a unified select-like interface to users of the RadioButtonGroup.  The current value
  semantics are preserved; other clients of the RadioButton do not see a change in behavior.

- EventEmitter: If the custom event detail is *not* an object, do not use the object-like semantics
  for forwarding it; just send it as-is.

- Comments: In the course of laying the groundwork for the application wizard, I throw a LOT of
  comments into the code, describing APIs, interfaces, class and function signatures, to better
  document the behavior inside and as signposts for future work.

* web: permit arrays to be sent in custom events without interpolation.

* actually use assignValue or rather serializeFieldRecursive

Signed-off-by: Jens Langhammer <[email protected]>

* web: eslint & prettier fixes, plus small aesthetic differences.

* Restoring this file.  Not sure where it disappears to.

* fix label in dark mode

Signed-off-by: Jens Langhammer <[email protected]>

* SCIM Manuel -> SCIM

Signed-off-by: Jens Langhammer <[email protected]>

* fix lint errors

Signed-off-by: Jens Langhammer <[email protected]>

* web: better converter configuration, CSS repair, and forward-domain-proxy

1. Forward Domain Proxy.  I wasn't sure if this method was appropriate for the wizard,
   but Jens says it is.  I've added it.

2. In the process of doing so, I decided that the Provider.converter field was overly
   complexified; I tried too hard to reduce the number of functions I needed to define,
   but in the process outsourced some of the logic of converting the Wizard's dataset
   into a property typed request to the `commit` phase, which was inappropriate.  All
   of the logic about a provider, aside from its display, should be here with the code
   that distinguishes between providers.  This commit makes it so.

3. Small CSS fix: the fields inherited from the Proxy provider forms had some unexpected
   CSS which was causing a bit of a weird indent.  That has been rectified.

* web: running pre-commit after merge.

* web: ensure the applications wizard tests finish after current changes

* prettier has opinions.

* web: application wizard spit & polish

The "ApplicationWizardHint" now correctly uses the localstorage and allows the user to navigate back
and see the message after it's been hidden, so that it will always be available during the test
phase.

The ApplicationList's old "Create Application Form" button has been restored for the purposes of the
test phase.

The ApplicationWizard is now available on both the ApplicationList and ProviderList pages.

Tana and I discussed the microcopy, putting a stronger second-person "You can do..." twist onto the
language, to give the user the sense of empowerment.

The ShowHintController now has both "hide" and "show" operations, to support the hint restoration.

* web: updated storybook stories for the wizard, illustration how "a simple wizard" is configured in source code and tested with storybook.

* web: I hate getting spanked by prettier.

* web: sometimes I wish I had lower standards

Anyway, this was a very stupid bug, because by definition function
definition arguments don't have uses, they're being defined, not
implemented.  Fixed, conf fixed to compensate, and consequences
conquered.

* move context from labs to main

Signed-off-by: Jens Langhammer <[email protected]>

* Revert "move context from labs to main"

This reverts commit 3718ee6.

* web: reify the data loop

I was very unhappy with the "update this dot-path" mechanism I was using earlier; it was hard
for me to read and understand what was happening, and I wrote the darned thing.  I decided instead
to go with a hard substitution model; each phase of the wizard is responsible for updating the
*entire* payload, mostly by creating a new payload and substituting the field value associated
with the event.

On the receiver, we have to do that *again* to handle the swapping of providers when the user
chooses one and then another.  It looks clunky, and it is, but it's *legible*; a junior dev
could understand what it's doing, and that's the goal.

* Revert "web: reify the data loop"

This reverts commit 09fedca.

* web: revert the 'lit' to 'lit-labs' for task and context.

---------

Signed-off-by: Jens Langhammer <[email protected]>
Co-authored-by: Jens Langhammer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants