ember-bootstrap / ember-bootstrap

Ember-cli addon for using Bootstrap as native Ember components.
https://www.ember-bootstrap.com
Other
492 stars 195 forks source link

Decouple BSForm's @model into an inital state and a list of changes #1551

Open jelhan opened 3 years ago

jelhan commented 3 years ago

The <BsForm> component expects an object, which represents the form state as @model argument. The component applies any user input on that object directly. This means <BsForm> mutates the object passed in.

This API served us very well over a long time. But it starts to feel off to me in modern Ember:

  1. Ember is heading more and more towards one-way data binding only. There are still some components, which mutate state passed in. The built-in <Input> component is one of them. But it is more and more getting an exception.
  2. Ember recently added a deprecation for mutating properties on an object returned by {{hash}} helper. Due to this {{hash}} helper can not be used anymore to create a form model in template. A consumer need to either create it in JavaScript or introduce a custom helper.
  3. Applying all changes to a form's model immediately is often not what the consumer wants. Changes need to be buffered often. Global state (e.g. Ember Data's store) should not be changed before user commits the changes explicitly (e.g. by clicking a submit button). Currently consumers either need to manage a fork of the state themselves or use a buffered proxy like Ember Changeset to achieve this.
  4. Not buffering changes complicates the implementation of a an abort or reset button. In practice if often requires change tracking, which is only supported for attributes but not for relationships by Ember Data.

All of these problems can be fixed. Community came up with great patterns and awesome addons to solve them. But I think we can do better by following the simple rule of not mutating state passed in.

I like to propose two changes to <BsForm>'s API for form state management:

  1. The state passed in is not mutated. It is considered an initial state. <BsForm> keeps track of the user input, the changes, separately.
  2. <BsForm> provides a list of changes to the consumer instead of the current form state (@model with changes applied).

More in detail the following APIs would be added or changed on <BsForm>:

Let me illustrate with an usage example:

<BsForm
  @initialState={{this.initialState}}
  @onSubmit={{this.login}}
  as |form|
>
  <form.element
    @controlType="E-Mail"
    @property="email"
    as |el|
  >
    {{#if (or form.changes.email (not this.initialState.email)}}
      <el.control @placeholder={{this.initialState.email}} />
    {{else}}
      You are currently logged in as {{this.initialState.email}}.
      <button {{on "click" (fn el.setState "")}}>
        Login as another user
      </button>
    {{/if}}
  </form.element>

  <form.element
    @controlType="Password"
    @property="password"
  />

  <form.submitButton>Login</form.submitButton>
</BsForm>
export default class LoginComponent extends Component {
  get initialState() {
    return {
      email: window.localStorage.getItem('last-used-email-address'),
    };
  }

  @action
  login(userInput) {
    const email = userInput.email ?? this.initialState.email;
    const { password } = userInput;

    if (!email) {
      window.alert('Please enter an email address');
    }

    if (!password) {
      window.alert('A password is required');
    }

    window.localStorage.setItem('last-used-email-address');

    // perform login logic
  }
}

<BsForm> might provide current form state as well. The current form state would be initial form state with user input applied. This would make it easier for consumers to accomplish some use cases. But it would require a proxy solution, which might introduce a lot of complexity. Therefore I would recommend to not do it in the first iteration. We could still add later.

Some applications have many forms. Refactoring all forms at once might be very challenging for them. I think we should support an incremental migration. That means we should introduce the new API before deprecating (or even removing) the old one. The presence of either @model or @initalState arguments could be used to distinguish between old and new API.

Both official validation plugins, ember-bootstrap-cp-validations and ember-bootstrap-changeset-validations rely on mutating form's @model currently. The validation is applied to the form's model before passing it as @model. The validation plugins overwrite <BsForm> and <BsForm::Element> to look up the validation state on that object.

The current validation API has several issues. I think we should not build the future of <BsForm> around it. Instead we should modernize its validation API in the same go. I have some ideas in mind, how that could look like. But I think we should discuss in a separate issue to not loose focus. Will try to open an issue to discuss that one in the next days.

simonihmig commented 3 years ago

Thanks for starting this, there is a lot of good stuff here!

I would be very interested in your thoughts on the validation API. Especially as mutating the @model was indeed the way to go to support ember-cp-validations, which was back then the primary validation library in Ember-land. Nowadays I see a hole in the Ember ecosystem regarding validations, with ember-cp-validations not fitting really into the Octane world, and ember-changeset-validations coupled to the concept of changesets.

Having said that, I felt a bit afraid even touching e-b's form API, as that seemed like going into a potentially really long rabbit hole, having to rethink the whole form api, with better composability and customizability (which it lacks currently IMHO), modern validations and what not...

Still I would think ideally one would tackle this space by working on a generic form library for Ember, that provides behavioral components (or other primitives), and that ember-bootstrap could use internally (transparently), or provide simple presentational components that just render the Bootstrap markup and can be used in conjunction with that generic library. After all 99% of the form functionality is not really related to Bootstrap, the rest is just adding the expected classes/markup.

This would also allow to experiment more freely what the ideal APIs would be, without worrying about breaking changes and consequences of adding new liabilities to our semver contract here.

On the other hand - again - I am just worried that trying to tackle this much bigger project asks too much of our available resources...

simonihmig commented 3 years ago

Forgot to mention in my previous comment the example of Formik, which seems very popular in React-land, which supports behavior, but does not render markup by itself. The latter is supported by "bindings" for opinionated rendering like Bootstrap. At least that's my superficial understanding of it.

Even if not trying to do something like "Formik for Ember", looking at its API (which of course cannot be translated to Ember literally) might be helpful?