1.0.0 Rewrite

[alexlafroscia]

Sorry to drop a massive PR bomb, but I've been talking to @pzuraq about these changes and wanted to deliver it all at once since it's all pretty tightly related. This encompasses a lot:

• Housekeeping stuff
  • Updating to the latest ember-cli blueprint
  • Adding prettier for consistent formatting
• Babel stuff
  • Babel 7 compatibility
  • Stage 2 decorators compatibility (so this package now matches the rest of the ember-decorators ecosystem)
• Implementation updates
  • Implementing the new decorator semantics, as described in Update to be a pure validation library, add TS support #83
  • Removing all of the configuration options, other than optionally keeping the code in production builds

A few other bits I'd like to call out that I'm a little proud of

• No longer requiring reopenClass anywhere, in favor of the supported ability to extend return a new class from the decorator finisher. This library could easily be totally decoupled from Ember with just a little tweaking to the timing of the validations
• No more mixins, for the same reason above; we can use actual ES6 class inheritance to inject the additional behavior where needed

What is not included

TypeScript Support. This is built on top of the Stage 2 decorator proposal, like the other decorators, so I think it would be better to wait until TypeScript also supports the Stage 2 decorators (from what I can tell, it currently does not). I am planning to convert this addon to TypeScript itself once the decorator compatibility story is figured out.

The immutable concept. required is handled by more specific types, but immutable is not. I am interested in re-introducing it through as a feature in the future, though, but as part of additional options to @argument rather than additional decorators to apply.

Todo (Maybe)

• Add programmatic tests around the production build stripping
  • I tested locally to make sure that it works, but a test that actually performs a production build and greps both the vendor and dummy files for any references to @ember-decorators/argument would probably be a good smoke test to ensure that that all works correctly going forward.

Closes #75
Closes #83 (Sort of)

[alexlafroscia]

3.4.0 selected as the minimum Ember version, because the new constructor/init behavior can be polyfilled back to that version

[alexlafroscia]

I added the context above, but want to call it out explicitly here, too:

I wrestled with this for a few hours to try to understand what's going on and why it didn't work. I think it comes down to this sequence of events:

• @argument('number') prop has the behavior of running Object.defineProperty at the end of the constructor
• The Object.defineProperty does not invoke the parent class getter or setter; the property is defined, not set. Because of this, the native getter and setter are entirely ignored
• prop is interpreted, during initial validation, to be undefined and thus fails the check

This has nothing to do with the code in this library; I validated all of this behavior in a fresh Babel project to understand what is happening.

I think the best course of action is just to not support this use-case at all, because anyone that has a getter and a property initializer is going to run into this same problem, @argument or not. I'm not sure what we could do to attempt to avoid the problem, but would love to re-enable this test at some point if we can figure out the right way to go about it. I left the test as a skip as an explicit reminder of this behavior.

[alexlafroscia]

Test failures seem to be because of a difference between how my local version of Chrome and the version on Travis read the class name after extending it with the validation logic. I'll look into making the assertions more flexible (or more consistently reporting the class name).

[pzuraq]

Overall this is looking great! I added some general comments. My last question is, do we want to allow users who don't want to deal with types to use @argument just to declare what argument values exist?

class FooComponent extends Component {
  @argument foo;
}

I think this would be nice for folks who want simpler validations. This could definitely be added after v1 though.

[pzuraq]

Should subclasses be able to narrow types?

class Foo {
  @argument('any') foo;
}

class Bar extends Foo {
  @argument('string') foo;
}

[alexlafroscia]

Hmm... that's interesting. We'd need to introduce a kind of hierarchy between types, right?

• 'object' => shapeOf({ what: 'ever' })
• 'array' => arrayOf('string')
• 'any' => literally anything else

I wonder what the real-world use-cases around something like that are. Have you seen a lot of people subclassing components? I personally haven't seen, since there isn't a good inheritance story around templates.

In a more general sense, I think they probably should be able to narrow types, but maybe that would be something to explore after the 1.0.0 release.

[pzuraq]

The way it originally worked was that we validated against all validators in the inheritance tree, if I remember correctly. So, in the above case, you would validate against both any and string. This would prevent incoherent validations:

class Foo {
  @argument('number') foo;
}

class Bar extends Foo {
  @argument('string') foo;
}

Would always fail one or the other, so users would never be able to pass in foo. Hard to add a good error message here though, so maybe this is too complicated and we should just either allow children to override parents, or keep the current behavior.

[alexlafroscia]

Ah, that's interesting. If you'd rather have parents be able to override the type, we can definitely do that instead.

[pzuraq]

I was still getting used to native classes at the time of writing this. I think there are still benefits to passing an object of "named params" through, but I have a feeling it'll be problematic for TS if we ever switch. Either way, not a big deal at the moment, just adding context here

[pzuraq]

I'm a bit hesitant about this, since we're adding a wrapper to each instance of the class. Definitely think this could affect perf at scale. Even though this is not done in prod, at some point it'll become problematic even in dev.

We can iterate on this, but I think ideally we would decorate the getters themselves in place so we only need to define the properties once.

[alexlafroscia]

That's a good point -- would you be OK with that being work that's done after this PR lands? It's massive as is, and I'd love to land it with a passing test suite and then work on refactoring a few things that won't break the public API, but can reduce the amount of code or improve performance.

Happy to do those things before we cut a 1.0.0 too

[pzuraq]

Yeah, totally cool to work on it after this PR, as long as it gets done before 1.0.0 I think. That would be the last chunk of really necessary work before release.

[alexlafroscia]

Sure thing! Maybe we can cut a beta release for people that are ready to start moving to this before that work happens.

[pzuraq]

👍

[alexlafroscia]

WRT allowing @argument to be used without a type at all -- I'm not sure. I kind of like the fact that there is just one way to use the decorator now.

@argument vs. @argument('any') would be the same now, and the latter is a little more explicit about your intention -- much like implicit vs. explicit any in TypeScript, I think it's probably better to guide people toward being explicit.

At least in our codebase, we've gotten in the habit of annotating the type for every argument to components, and it's been a huge help when refactoring. I like the idea of gently encouraging people to at least be a little specific about what their arguments are supposed to be.

Another thought: if we bring back the immutable flag, I was imagining the API would look something like this:

@argument('string', { immutable: true })

If we allow the decorator without a type, then all of these become valid:

@argument
@argument('string')
@argument({ immutable: true })
@argument('string', { immutable: true })

Which isn't necessarily bad, but is just an avenue for inconsistency that we can opt to steer away from if we'd prefer.

[pzuraq]

👍 Yeah that's fair, let's continue with it for now being explicit and we can see how people use it in the wild. To be fair, if anyone really wanted that behavior, they could just do:

import argument as originalArgument from '@ember-decorators/argument';

export const argument = argument('any');

Somewhere locally and they have it

[josemarluedke]

Awesome refactor here.

Question, for someone using the option { defaultIfUndefined: true }), how should someone proceed on upgrading to this version?

[alexlafroscia]

I'm pretty sure that that behavior is just how the decorators work now -- if you provide a default value, it will be used unless the argument is passed in.

@josemarluedke have you tried upgrading to the beta version and hit an issue?

@pzuraq was there more that defaultIfUndefined was doing?

[pzuraq]

Yes, defaultIfUndefined was doing two things that were different:

1. If the value passed to the component was undefined, it would still override it. This is something that is pretty common in components that are wrapping other components:

{{!-- outer-component.hbs --}}
{{inner-component foo=foo}}

{{!-- outer-component.hbs --}}
{{outer-component}}

In this case, you either need to define logic to handle the case where an undefined value is passed to inner-component, or you need to redefine defaults in outer-component which is a decent chunk of boilerplate (sidenote: I think this would be better handled with splarguments syntax in the future {{inner-component ...arguments}})

2. The defaulting behavior in defaultIfUndefined would check to see if the value was undefined every time it was assigned. This handles the case where a value may initially be passed to the component, but is then changed to undefined later on. I think this use case is less common though.

I think there's room for these behaviors to be their own set of decorators in a separate library, leaving @argument to focus solely on validation.

[alexlafroscia]

I see, thank you for that clarification!

We might want to do something about that first use-case... that feels pretty important. We can always go back to allowing a set of options to @argument

@argument('string', { defaultIfUndefined = true })

@josemarluedke which of those features were you relying on? Asking to better understand the use-case and how we want to proceed here.

[josemarluedke]

I came across this option while trying to upgrade ember-table to use the beta version: Addepar/ember-table#646

[alexlafroscia]

Awesome, that's great context to have.

Since the idea with this package is that it's completely removed in production builds, I think that the defaultIfUndefined behavior is at odds with that goal. @pzuraq is right in that we should probably introduce a separate add-on that provides that behavior.

@pzuraq: are you interested in creating an additional ember-decorators add-on that includes this behavior? It think we should define this migration path before publishing 1.0.0

I'm imagining something like this

import { defaultValue } from '@ember-decorators/default-value';

class Whatever extends Component {
  @defaultValue({ ifUndefined = true, ifNull = true })
  prop = 'false'
}

I don't really like the specific names I use there, but an API similar to that that includes the old runtime behavior from this package. You could then apply the type validation and default behavior decorators independently.