Skip to content
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

Streamline directive definitions with more liberal use of decorators #1800

Closed
ewinslow opened this issue May 11, 2015 · 12 comments
Closed

Streamline directive definitions with more liberal use of decorators #1800

ewinslow opened this issue May 11, 2015 · 12 comments
Assignees
Labels
effort3: weeks feature Issue that requests a new feature state: Needs Design

Comments

@ewinslow
Copy link

According to the docs, making a directive that listens for events on the host element is like so:

@Directive({
  selector: 'input',
  hostListeners: {
    'change': 'onChange($event)',
    'window:resize': 'onResize($event)'
  }
})
class InputDirective {
  onChange(event:Event) {}
  onResize(event:Event) {}
}

This seems like it could be streamlined:

  • The listeners are registered far away from where the handlers are defined
  • The registration duplicates a) the method names and b) arguments.
  • The registration uses strings which makes obfuscation and refactoring more difficult.
  • You can listen for global events, too (seems odd that the hostListener object lets you do this)
  • Listening for global events depends on a magic string prefix of "window:" (or "document:" or "body:", implied by the docs)

Here's a possible alternative:

@Directive({
  selector: 'input'
})
class InputDirective {
  @HostListener('change')
  onChange(event: Event) {}

  @WindowListener('resize')
  onResize(event: Event) {}
}

This doesn't suffer from any of the problems listed above.

You can imagine a similar streamlining for properties. Currently you write:

@Directive({
  selector: '[dependency]',
  properties: {
    'id':'dependency'
  }
})
class Dependency {
  id:string;
}

But using decorators you could write:

@Directive({
  selector: '[dependency]',
})
class Dependency {
  @Property('dependency')
  id:string;
}

I find it a whole lot easier to understand this, not needing to remember constantly what the left and right hand side of the configuration means. E.g. for { 'id': 'dependency' } I cannot tell easily which one of 'id' or 'dependency' is the JS class property and which is the DOM element property. I constantly get this confused in Angular 1, too, with the scope config. Hopefully we can eliminate this rough edge in Angular 2...

@aciccarello
Copy link
Contributor

I like the way that looks. I've been a little concerned about having a huge annotation before the class definition. This would be a good way to tie things together. I haven't looked at the decorators/annotations spec yet but I hope they're available on a method level.

@aciccarello
Copy link
Contributor

Just noticed a similar suggestion with injectables on #1451 though I see you've already found that

@pkozlowski-opensource
Copy link
Member

I think that there is some confusion going on regarding #1451 as it mixes up configuration of the DI container with specifying what should be injected from the previously configured DI container. In this example:

@Component()
class MyComponent {
  @Inject(SomeServiceImpl) 
  constructor(someService:SomeServiceImpl) {}
}

You don't really need @Inject(SomeServiceImpl) - type annotation is enough. But you still need to say what is available in the DI container (this is what injectables does in @component) and using @Inject name for this doesn't make sense

@ewinslow
Copy link
Author

Type annotation is enough only if you type on a concrete class. Agree that in that case any other info should be unnecessary. That would certainly be nice.

@Inject may be a poor name. I'm sure we can come up with a better one. Maybe @Injectables? It could take an array. This would be decoupled from Component. Seems like a positive to decouple injector configuration from Component configuration actually. WDYT?

@mhevery
Copy link
Contributor

mhevery commented Jun 1, 2015

This is planned as a short hand for putting everything in @Component

@ewinslow
Copy link
Author

ewinslow commented Jun 2, 2015

Woo hoo!

@pkozlowski-opensource
Copy link
Member

Actually, after creating few components I'm more and more warming up to the idea of using annotations to denote properties and events. Ex, instead of:

@Component({
    selector: 'bs-alert',
    properties: ['type', 'dismissible'],
    events: ['dismiss']
})
class BsAlert {
   dismiss = new EventEmmiter();    
   set type(val) {...}
}

I would be cool if I could write:

@Component({
    selector: 'bs-alert'
})
class BsAlert {
   @Event()
   dismiss = new EventEmmiter();    

  @Property()
   set type(val) {...}
}

The biggest advantage for me would be that I don't have to duplicate property / event names

@PatrickJS
Copy link
Member

👍 for alternative decorator annotations

@dsebastien
Copy link

+1 for @event() & @Property(); feels natural/explicit/intuitive

@vsavkin
Copy link
Contributor

vsavkin commented Sep 10, 2015

Fixed 896add7

@vsavkin vsavkin closed this as completed Sep 10, 2015
@ewinslow
Copy link
Author

Woot!

kegluneq pushed a commit to kegluneq/angular that referenced this issue Sep 11, 2015
Update the transformer to generate code registering annotations on class
properties, getters, and setters.

Closes angular#1800, angular#3267, angular#4003
kegluneq pushed a commit to kegluneq/angular that referenced this issue Sep 14, 2015
Update the transformer to generate code registering annotations on class
properties, getters, and setters.

Closes angular#1800, angular#3267, angular#4003
kegluneq pushed a commit to kegluneq/angular that referenced this issue Sep 15, 2015
Update the transformer to generate code registering annotations on class
properties, getters, and setters.

Closes angular#1800, angular#3267, angular#4003
kegluneq pushed a commit that referenced this issue Sep 16, 2015
Update the transformer to generate code registering annotations on class
properties, getters, and setters.

Closes #1800, #3267, #4003
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort3: weeks feature Issue that requests a new feature state: Needs Design
Projects
None yet
Development

No branches or pull requests

7 participants