Skip to content

refactor(LifecycleEvent): remove LifecycleEvent - #3928

Closed
vicb wants to merge 4 commits into
angular:masterfrom
vicb:0831-lifecycle
Closed

refactor(LifecycleEvent): remove LifecycleEvent#3928
vicb wants to merge 4 commits into
angular:masterfrom
vicb:0831-lifecycle

Conversation

@vicb

@vicb vicb commented Sep 1, 2015

Copy link
Copy Markdown
Contributor

TODO:

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

@vicb vicb added the state: WIP label Sep 1, 2015
mhevery
mhevery reviewed Sep 1, 2015
View reviewed changes
Comment thread modules/angular2/src/core/compiler/interfaces.ts Outdated

@mhevery mhevery Sep 1, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you have to make these into classes?

@vicb vicb Sep 1, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

vicb commented Sep 1, 2015

Copy link
Copy Markdown
Contributor Author

@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
  // ...
}
  1. Could you please check if the updates to transform/directive_processor/visitors.dart & tests are ok,
  2. 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

@vicb
vicb force-pushed the 0831-lifecycle branch 2 times, most recently from 375cd46 to b2aa2b1 Compare September 1, 2015 06:49
kegluneq
kegluneq reviewed Sep 1, 2015
View reviewed changes
Comment thread modules/angular2/src/core/compiler/directive_lifecycle_reflector.dart Outdated

@kegluneq kegluneq Sep 1, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is preventing us from providing types for these parameters?

@kegluneq

kegluneq commented Sep 1, 2015

Copy link
Copy Markdown

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

vicb commented Sep 1, 2015

Copy link
Copy Markdown
Contributor Author

@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 !

@vicb vicb mentioned this pull request Sep 2, 2015
Closed
@kegluneq

kegluneq commented Sep 2, 2015

Copy link
Copy Markdown

/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

keertip commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

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

@vicb
vicb force-pushed the 0831-lifecycle branch 2 times, most recently from 7ad45d6 to f133d39 Compare September 2, 2015 20:44
@vicb vicb changed the title [WIP] refactor(LifecycleEvent): remove LifecycleEvent refactor(LifecycleEvent): remove LifecycleEvent Sep 2, 2015
@vicb vicb assigned mhevery and unassigned kegluneq Sep 2, 2015
@vicb
vicb force-pushed the 0831-lifecycle branch 3 times, most recently from 7a4e2a7 to 815a0bf Compare September 3, 2015 00:51
@vicb

vicb commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

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

@vicb vicb mentioned this pull request Sep 3, 2015
Closed
@vicb
vicb force-pushed the 0831-lifecycle branch 2 times, most recently from 7edbad7 to 3689bae Compare September 4, 2015 00:26
@vicb

vicb commented Sep 4, 2015

Copy link
Copy Markdown
Contributor Author

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

@mhevery could you please review.

@mhevery mhevery assigned vicb and unassigned mhevery Sep 4, 2015
@vicb vicb added the action: merge The PR is ready for merge by the caretaker label Sep 4, 2015
@mary-poppins

mary-poppins commented Sep 4, 2015

Copy link
Copy Markdown

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

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

mary-poppins commented Sep 4, 2015

Copy link
Copy Markdown

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

@mary-poppins 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
@vicb
refactor(LifecycleEvent): remove LifecycleEvent
ecfed76 
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() {
        }
      });
Tim Blasi @vicb
refactor(transformers): extract lifecycle hooks from the interfaces
f6449f3
@vicb
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 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 vicb closed this in 86bda28 Sep 5, 2015
@yjbanov

yjbanov commented Sep 5, 2015

Copy link
Copy Markdown
Contributor

/cc @jeffbcross

@jeffbcross

jeffbcross commented Sep 5, 2015

Copy link
Copy Markdown
Contributor

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

@vicb

vicb commented Sep 8, 2015

Copy link
Copy Markdown
Contributor Author

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

@jeffbcross

jeffbcross commented Sep 8, 2015

Copy link
Copy Markdown
Contributor

Haha @vicb no worries

@vicb
vicb deleted the 0831-lifecycle branch September 29, 2015 23:38
@angular-automatic-lock-bot

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

Copy link
Copy Markdown

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.

Reviewers

No reviews

Assignees

@vicb vicb

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@vicb @kegluneq @keertip @mary-poppins @yjbanov @jeffbcross @mhevery @googlebot