clients icon indicating copy to clipboard operation
clients copied to clipboard

[EC-558] refactor `appApiAction` (add new alternative)

Open coroiu opened this issue 2 years ago • 3 comments

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Refactor the older appApiAction directive, modernize it to use observables and add it to the CL.

Usage example

Standard method

<form [formGroup]="formGroup" (ngSubmit)="submit()">
  <button type="submit" bitButton [loading]="loading">
    {{ "submit" | i18n }}
  </button>
</form>
class Component {
  loading = false;
  formGroup = this.formBuilder.group({...})
  
  async submit() {
    this.loading = true;
    try {
      // [... submit logic ...]
    } catch (err) {
      this.logService.log(err);
    } finally {
      this.loading = false;
    }
  }
}

appApiAction method

<form #form (ngSubmit)="submit()" [appApiAction]="formPromise">
  <button type="submit" bitButton [loading]="form.loading">
    {{ "submit" | i18n }}
  </button>
</form>
class Component {
  formPromise: Promise<unknown>;
  formGroup = this.formBuilder.group({...})
  
  async submit() {
    try {
      this.formPromise = this.doSubmit();
      await this.formPromise;
    } catch (err) {
      this.logService.log(err);
    }
  }
  
  private async doSubmit() {
    // [... submit logic ...]
  }
}

(New in this PR) bitSubmit method

<form [formGroup]="formGroup" [bitSubmit]="submit">
  <button type="submit" bitButton bitFormButton>
    {{ "submit" | i18n }}
  </button>
</form>
class Component {
  formGroup = this.formBuilder.group({...})
  
  //note: submit can also return Observable instead of Promise
  submit: async () => {
    try {
      // [... submit logic ...]
    } catch (err) {
      this.logService.log(err);
    }
  }
}

Code changes

  • bit-submit.directive: New directive that attaches to a form tag with [formGroup] directive. Most of the magic happens here. The directive attaches to a form, extracts the FormGroupDirective and hooks into ngSubmit. When the user submits, BitSubmitDirective picks up the action and triggers the developer-supplied async submit handler. The status of the async execution is exposed using the public loading$ observable field. Note: If the submit handler returns an Observable it will automatically be unsubscribed and aborted if the UI is destroyed.

  • form-button.directive: A layer in-between bitButton and bitSubmit. It subscribes to BitSubmitDirective.loading$ and sets BitButtonDirective.loading accordingly. This results in the button automatically disabling and showing loading state when the async submit handler is running.

  • submit-button.component: A convenient component that can be used as a shorthand for

  <button type="submit" bitButton bitFormButton>
    {{ "submit" | i18n }}
  </button>

Screenshots

See storybook

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

coroiu avatar Sep 16 '22 06:09 coroiu

Comment from @Hinton in https://github.com/bitwarden/clients/pull/3537

I think it's a good refactor, it will certainly let us get rid of the annoying form promises we create everywhere. I'm unsure of the BitFormButtonDirective, is the thinking that we want to disable all buttons during a form submit? If so we probably don't want all of them to display the loading effect but rather disable them, with the submit button having the effect.

Yeah you're probably right about that, cancel buttons should probably not have the spinner. But if everything else looks good then I'd gladly but some more effort into this and fix that issue!

coroiu avatar Sep 16 '22 08:09 coroiu

As we discussed this seems like a nice improvement. We'll want to rework this so that buttons can have their own handlers if they aren't the main submit button. And the other things we discussed like inverting the relationship and have buttons check for the action.

Hinton avatar Sep 16 '22 15:09 Hinton

Currently waiting to merge base branch, will then update this and prepare it for review

coroiu avatar Sep 20 '22 12:09 coroiu

I do think we need to document this thoroughly. What are your thoughts on adding descriptions to each story + some comments to the directives, and writing a markdown help page https://storybook.js.org/docs/react/writing-docs/mdx#documentation-only-mdx

We should also spend a few minutes thinking through how we want to organize the storybook pages for these components, since they are all inter-connected, and having them be spread out will be confusing.

@Hinton I agree with both of these, I can add the documentation! Do you have any suggestion on the organizational structure? I have no idea what would be better

coroiu avatar Sep 29 '22 12:09 coroiu

This is really nice, amazing work! 🎉 Any chance you can swap over an existing use of appApiAction to these new directives? Maybe one standalone button, and a submit case? Your usage examples are great, but it'd be nice to see these being used in action 😎

differsthecat avatar Oct 06 '22 19:10 differsthecat