refactor(LifecycleEvent): remove LifecycleEvent by vicb · Pull Request #3928 · angular/angular

vicb · 2015-09-01T04:43:30Z

TODO:

dart metadata (transformer should extract data from interfaces)
doc Directive & Component should link to lifecycle hooks

mhevery · 2015-09-01T05:06:54Z

modules/angular2/src/core/compiler/interfaces.ts

Why did you have to make these into classes?

So that we can write hasLifeCycleHook(OnChanges, MyComponent); and don't have to maintain a duplicate enum.

vicb · 2015-09-01T06:19:21Z

@kegluneq I would appreciate some help to update the Dart transformers for this PR - I have done some updates but more is needed.

The PR is about removing the need for declaring lifecycle: const[<list of hooks>] and only rely on the interfaces implemented by the underlying class:

@Component({
   // lifecycle: const [LifecycleEvent.OnChanges], -> This config is dropped
   ...
})
class MyComp implements OnChanges { // -> The code now relies on the interfaces which already are the type info
  // ...
}

Could you please check if the updates to transform/directive_processor/visitors.dart & tests are ok,
src/transform/common/directive_metadata_reader.dart should be updated to extract the information from the implemented interfaces instead of from the lifecycle config.

For item 2 above, the best would be if you could send a PR to my repo: vicb/angular

kegluneq · 2015-09-01T16:37:51Z

modules/angular2/src/core/compiler/directive_lifecycle_reflector.dart

What is preventing us from providing types for these parameters?

kegluneq · 2015-09-01T16:54:45Z

I don't see any issues with the changes so far, with the exception of your point (2) above, that RenderDirectiveMetadata currently won't be populated.

I would guess that is why the Travis test is failing.

I can make the updates to directive_metadata_reader, but I'm not certain I'll be able to do it today. I don't think there's anything particularly tricky about the change, so if you're in a hurry feel free to give it a shot.

The current implementation expects to be processing either the Directive annotation on a class declaration or the (similar) InstanceCreationExpression for the same annotation that is generated for the .ng_deps.dart files. The problem is that InstanceCreationExpression does not have an associated ClassDefinition from which we can pull interfaces.

I think the easiest way to make this update is to have the reader accept, instead of the InstanceCreationExpression for the annotation class, the InstanceCreationExpression for the ReflectionInfo class, which specifies both the annotation(s) and the interface(s) we need.

vicb · 2015-09-01T20:43:29Z

@kegluneq you're right, directive_metadata_reader is most probably the root cause.

You would be much more efficient than me to do the updates there. So please take a look whenever you have time.

Thanks !

kegluneq · 2015-09-02T19:57:40Z

/cc @keertip this change required updates to directive_metadata_reader, which may cause some issues if it's still used to read RenderDirectiveMetadata in the plugin

Let me know if you see any issues stemming from this.

keertip · 2015-09-02T20:16:47Z

@kegluneq , thanks for the info. Will take a look after this lands.

vicb · 2015-09-03T00:53:10Z

The last commit triggers a typings error, not sure how to solve this, see https://travis-ci.org/angular/angular/jobs/78499681#L260

vicb · 2015-09-04T01:10:30Z

Thanks to @IgorMinar the typings generation issue has been solved.

@mhevery could you please review.

mary-poppins · 2015-09-04T21:45:34Z

Merging PR #3928 on behalf of @vicb to branch presubmit-vicb-pr-3928.

mary-poppins · 2015-09-04T22:41:18Z

Merging PR #3928 on behalf of @vicb to branch presubmit-vicb-pr-3928.

fixes angular#3924 BREAKING CHANGE The `lifecycle` configuration for directive has been dropped. Before // Dart @component({lifecycle: const [LifecycleEvent.OnChanges], ...}) class MyComponent implements OnChanges { void onChanges() {...} } // Typescript @component({lifecycle: [LifecycleEvent.OnChanges], ...}) class MyComponent implements OnChanges { onChanges(): void {...} } // ES5 var MyComponent = ng. Component({lifecycle: [LifecycleEvent.OnChanges], ...}). Class({ onChanges: function() {...} }); After // Dart @component({...}) class MyComponent implements OnChanges { void onChanges() {...} } // Typescript @component({...}) class MyComponent implements OnChanges { onChanges(): void {...} } // ES5 var MyComponent = ng .Component({...}) .Class({ onChanges: function() { } });

…cle_hooks) BREAKING CHANGE Lifecycle hooks now live in the `angular2/lifecycle_hooks` module. They previously lived in the `metadata` module.

yjbanov · 2015-09-05T05:02:23Z

/cc @jeffbcross

jeffbcross · 2015-09-05T05:29:11Z

Thanks @yjbanov I've already (painfully) rebased against these changes :)

vicb · 2015-09-08T15:28:11Z

@jeffbcross sorry for that, I know how it feels ! - I had to rebase this one >5 times to get it in :(

jeffbcross · 2015-09-08T15:41:17Z

Haha @vicb no worries

angular-automatic-lock-bot · 2019-09-07T06:21:57Z

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.}

googlebot added the cla: yes label Sep 1, 2015

vicb added the state: WIP label Sep 1, 2015

mhevery reviewed Sep 1, 2015
View reviewed changes

mhevery assigned kegluneq Sep 1, 2015

vicb force-pushed the 0831-lifecycle branch 4 times, most recently from 68efb5a to 694ff70 Compare September 1, 2015 06:12

vicb force-pushed the 0831-lifecycle branch 2 times, most recently from 375cd46 to b2aa2b1 Compare September 1, 2015 06:49

kegluneq reviewed Sep 1, 2015
View reviewed changes

vicb mentioned this pull request Sep 2, 2015

Adde more LifecycleEvents #3863

Closed

vicb force-pushed the 0831-lifecycle branch from b2aa2b1 to 6815529 Compare September 2, 2015 19:47

vicb force-pushed the 0831-lifecycle branch 2 times, most recently from 7ad45d6 to f133d39 Compare September 2, 2015 20:44

vicb changed the title ~~[WIP] refactor(LifecycleEvent): remove LifecycleEvent~~ refactor(LifecycleEvent): remove LifecycleEvent Sep 2, 2015

vicb assigned mhevery and unassigned kegluneq Sep 2, 2015

vicb force-pushed the 0831-lifecycle branch 3 times, most recently from 7a4e2a7 to 815a0bf Compare September 3, 2015 00:51

vicb mentioned this pull request Sep 3, 2015

OnDestroy in a component without LifecycleEvent.OnDestroy #3985

Closed

vicb force-pushed the 0831-lifecycle branch 2 times, most recently from 7edbad7 to 3689bae Compare September 4, 2015 00:26

vicb force-pushed the 0831-lifecycle branch from b0c815d to 496aac3 Compare September 4, 2015 00:52

vicb force-pushed the 0831-lifecycle branch from 496aac3 to 4c01613 Compare September 4, 2015 21:23

mhevery added the pr_state: LGTM label Sep 4, 2015

mhevery assigned vicb and unassigned mhevery Sep 4, 2015

vicb added the action: merge The PR is ready for merge by the caretaker label Sep 4, 2015

mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Sep 4, 2015

vicb force-pushed the 0831-lifecycle branch from 9841588 to d782c7e Compare September 4, 2015 22:40

vicb added the action: merge The PR is ready for merge by the caretaker label Sep 4, 2015

mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Sep 4, 2015

vicb and others added 3 commits September 4, 2015 17:19

refactor(transformers): extract lifecycle hooks from the interfaces

f6449f3

refactor(Lifecycle hooks): move the hooks to their own module (lifecy…

dffc993

…cle_hooks) BREAKING CHANGE Lifecycle hooks now live in the `angular2/lifecycle_hooks` module. They previously lived in the `metadata` module.

vicb force-pushed the 0831-lifecycle branch from d782c7e to a70c928 Compare September 5, 2015 00:20

fix(DirectiveResolver): Synced with latest changes

f38c3ab

vicb force-pushed the 0831-lifecycle branch from a70c928 to f38c3ab Compare September 5, 2015 00:37

vicb added action: merge The PR is ready for merge by the caretaker and removed state: WIP action: merge The PR is ready for merge by the caretaker labels Sep 5, 2015

vicb closed this in 86bda28 Sep 5, 2015

vicb deleted the 0831-lifecycle branch September 29, 2015 23:38

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

Conversation

vicb commented Sep 1, 2015

Uh oh!

mhevery Sep 1, 2015

Choose a reason for hiding this comment

Uh oh!

vicb Sep 1, 2015

Choose a reason for hiding this comment

Uh oh!

vicb commented Sep 1, 2015

Uh oh!

kegluneq Sep 1, 2015

Choose a reason for hiding this comment

Uh oh!

kegluneq commented Sep 1, 2015

Uh oh!

vicb commented Sep 1, 2015

Uh oh!

kegluneq commented Sep 2, 2015

Uh oh!

keertip commented Sep 2, 2015

Uh oh!

vicb commented Sep 3, 2015

Uh oh!

vicb commented Sep 4, 2015

Uh oh!

mary-poppins commented Sep 4, 2015

Uh oh!

mary-poppins commented Sep 4, 2015

Uh oh!

yjbanov commented Sep 5, 2015

Uh oh!

jeffbcross commented Sep 5, 2015

Uh oh!

vicb commented Sep 8, 2015

Uh oh!

jeffbcross commented Sep 8, 2015

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants