ember-decorators / argument

Decorators for Component and Object arguments in Ember
MIT License
30 stars 18 forks source link

Glimmer Component API #95

Open buschtoens opened 5 years ago

buschtoens commented 5 years ago

The Glimmer Component API works different to the old Ember Component API. Arguments are not placed directly on the Component instance, but are set as the args object on the instance. This means that this addon is incompatible with Glimmer Components.

To get a feel for how they work, you can use sparkles-component.

The question I want to solve is: How can we expose the same validation system to Glimmer Components?

alexlafroscia commented 5 years ago

That's a really good question! I'd love to try to tackle making it work there.

I don't know a ton about the difference between sparkles-component and the direct use of Glimmer components. If I got this package working with sparkles-component, would it be safe to assume it would also work with Glimmer components at that time?

Thankfully, the way it's been re-written, it's almost at the point where it will work without EmberObject and init, so I don't think it should be too bad to make this work!

pzuraq commented 5 years ago

The APIs are similar enough that it would probably work with both of them, I'd say go for it 👍

alexlafroscia commented 5 years ago

Awesome. I'll play with that idea once I've had a chance to get the performance optimizations that we want to do

gossi commented 5 years ago

Any idea to "merge" in @arg from sparkles-decorators to provide defaults and being type-checked? Since arguments don't have a default, which was the idea for @arg.

It's not good to have @arg and @argument. I think it's also more of a question in which direction the framework is moving here.

buschtoens commented 5 years ago

@ember-decorators/argument has become a validation only library, that has no effect on semantics. The usage example showing a default value might be a bit misleading in that regard, since @argument does not provide any default behavior any more.

Since the native class RFC, this is native Ember behavior. It is also misleading in that, if you supply an undefined / null argument, it will actually override the default value. In the same vein, if an argument that was present gets updated to undefined / null, the default value will not be restored.

import Component from '@ember/component';
import { argument } from '@ember-decorators/argument';

export default class ExampleComponent extends Component {
  @argument('string')
  arg = 'default';

  didReceiveAttrs() {
    super.didReceiveAttrs();

    console.log(this.arg); // => undefined
  }
}
{{example-component arg=undefined}}

There's been talk about a @defaultIfUndefined / ...ifEmpty decorator or something with a similar name. See https://github.com/miguelcobain/ember-yeti-table/pull/17 for a crude implementation that is missing some key aspects.

For Glimmer components, what we'd need is something like @readsWithDefault.

import Component from 'sparkles-component';
import { argument } from '@ember-decorators/argument';

export default class ExampleComponent extends Component {
  args: {
    arg: string;
  };

  @argument('string')
  @readsWithDefault('args.arg')
  arg = 'default';

  didReceiveAttrs() {
    super.didReceiveAttrs();

    console.log(this.arg); // => 'default'
  }
}

Applying the decorators in that order would also eliminate the need for changes to @argument's validation system, if I am not mistaken. However, it does not look really ergonomic.

gossi commented 5 years ago

I see your point, makes sense. Having @arg and @argument is weird. But your semantics of @readsWithDefault() kinda imply (at least to me) that the default is given as parameter to the decorator.... I rested a little on this statement and tried thinking about just @read, when then ... it makes sense to have @readsWithDefault - but I guess it was also only a quick-suggestion by you, tricky naming beast :D

Given your examples, I question the "position" of the @argument decorator in your examples. Since they are on a property and properties aren't necessary mapped to arguments. I would put that onto args (also mostly only TS users will have the args written in there, because of typing purpose). If it would stick to the args property, then perhaps @validation is semantically more correct:

export default class ExampleComponent extends Component {
  @validation('foo', {type: 'string', required: true})
  args: {
    foo: string;
  };

  @readsWithDefault('args.foo')
  foo: string = 'bar';
}

For JS only users, it wouldn't make sense to have it there, then I would say class level is my next bet (... and going back to @argument):

@argument('foo', {type: 'string', required: true})
export default class ExampleComponent extends Component {
  @readsWithDefault('args.foo')
  foo = 'bar';
}

I also maybe played around with a different syntax for @argument/@validation ;) I think after all my comment is just loud brainstorming... not more.

alexlafroscia commented 5 years ago

Yeah, I think you bring up a really good point here @gossi -- the naming of this library has been on my mind too. Calling it @type or @validate might make more sense -- especially since a Validator is now, internally, something you could potentially extend yourself to validate whatever kind of logic you want. Thankfully, there's nothing stopping you from doing this right now:

import { argument as type } from '@ember-decorators/argument';

If that feels better in the meantime!

It might be worth considering re-naming this library before we hit 1.0.0 to be more inline with the current behavior which, as @buschtoens mentioned, is now validation-only with no behavioral side effects besides validating values in non-production builds (which is different than the original behavior).

pzuraq commented 5 years ago

To give some context here, part of the reason I wanted to scope this library down to focus on arguments specifically was because we were basically beginning to reinvent TypeScript with the @type decorators. In theory you can use them to check types anywhere, not just arguments, but it is tricky and a bit error prone, and really not as elegant as TS. My thinking was that focusing on component arguments, whose types can't currently be enforced, strikes a nice balance here, giving us some nice validations without going overboard.

With that in mind, the reasoning behind @argument vs @type was that it implies that scope. @type sounds like something you can apply anywhere - and people definitely will.

That said, if y'all want to go that direction, go for it 😄 I don't have enough time to actively contribute as much to this lib and I don't want to block experimentation and such.

willviles commented 5 years ago

I agree with @pzuraq. Seeing as this library is restricted to specifically type checking arguments, it makes sense that it implements a decorator named @argument, rather than a more nebulous @type or @validate.

My main concern is with finding a nice syntax for supplying default values to arguments. My codebases have been using the @argument foo = 'defaultValue' syntax since the 0.1.0 release of @ember-decorators/arguments and I've had absolutely no problems with that syntax.

For me, the best ergonomics of the @readsWithDefault functionality proposed by @buschtoens is applying it implicitly via the @argument decorator. I know the vision for this addon is to be validation-only, but this seems like the cleanest solution. Unless there's a good reason why that can't happen?

I'd also like to see @argument without any types passed as a lazy shorthand for 'any' - or at least some config to be able to allow this. This is simply for speed of development purposes, so I'm not bumping into an assertion because I haven't imported and assigned the right type for the argument when I'm just starting to sketch out the component's API.

evanb2 commented 4 years ago

Is there any plan to work on this enhancement now that Octane has been released?

This RFC had some great discussion but it sounds like any progress on this is a long ways out. Would love to see a solution here in the mean time.

alexlafroscia commented 4 years ago

From me, no, and I don’t think that @pzuraq is working on this package anymore either.

It’s not really clear how it would work, since Glimmer components access their arguments “directly” (rather than as a mix of external and internal state). Thinking about the this.args.FOO or @FOO syntax, there’s really no place to inject the logic around validating the values (unlike the Ember component API, where we could use a decorator to wrap the property and validate in a setter).

evanb2 commented 4 years ago

@alexlafroscia Thanks for the response. Super bummed, was really hoping to have something to replicate React's propTypes in the Ember-land.

evanb2 commented 4 years ago

Also, should probably add a note in the README about this. Especially if you're not planning on actively maintaining the package anymore.

pzuraq commented 4 years ago

Agreed that we should definitely add a disclaimer about this.

@evanb2 re: something like propTypes, I think we do actually have some possible paths forward. Part of the issue is we really need to figure out default props and imports, as I think that the design of both of these would influence the design of argTypes, but currently I'm thinking of something like:

const types = {
  foo: 'string',
  bar: 'number',
  baz: (value) => {
    // custom validation logic
  }
}; 

const defaults = {
  foo: 'hello',
  bar: 'world',
  baz: () => [] // init an array
}

<template>
  {{arg-types types}}
  {{arg-defaults defaults}}
</template>

This way we're minimizing the micro syntax required, and we have a solution that works in both TO and class components.

evanb2 commented 4 years ago

That'd be awesome. Though I would prefer doing something like:

const types = {
  foo: argTypes.string,
  bar: argTypes.number,
};

Again, like propTypes. But I also just really dislike the idea of using strings to define the types. Just my 2 cents.

buschtoens commented 4 years ago

Would {{arg-defaults}} actually set default values for @foo or a differently named local scope variable?

pzuraq commented 4 years ago

{{arg-defaults}} would need to set the actual value of the argument, both in the template and component js. I’m pretty sure it would require at the least a custom component manager, possibly would need to be built in.