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.

Sign up for GitHub

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

Jump to bottom

refactor(LifecycleEvent): remove LifecycleEvent #3928

Closed
wants to merge 4 commits into from
Closed

refactor(LifecycleEvent): remove LifecycleEvent #3928

wants to merge 4 commits into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Sep 1, 2015

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
modules/angular2/src/core/compiler/interfaces.ts
/**
* Defines lifecycle method {@link metadata/LifeCycleEvent#OnChanges `LifeCycleEvent.OnChanges`}
* called after all of component's bound properties are updated.
*/
export interface OnChanges { onChanges(changes: StringMap<string, any>): void; }
export class OnChanges implements LifecycleHook {
Copy link
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?

Copy link
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
Copy link
Contributor Author

vicb commented Sep 1, 2015

@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
modules/angular2/src/core/compiler/directive_lifecycle_reflector.dart
return annotation.lifecycle.contains(e);
} else {
if (type is! Type) return false;
bool hasLifecycleHook(/*LifecycleHook*/ interface, type) {
Copy link

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
Copy link

kegluneq commented Sep 1, 2015

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
Copy link
Contributor Author

vicb commented Sep 1, 2015

@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
Copy link

kegluneq commented Sep 2, 2015

/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
Copy link
Contributor

keertip commented Sep 2, 2015

@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
Copy link
Contributor Author

vicb commented Sep 3, 2015

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
Copy link
Contributor Author

vicb commented Sep 4, 2015

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
Copy link

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
Copy link

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() {
        }
      });
@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
Copy link
Contributor

yjbanov commented Sep 5, 2015

/cc @jeffbcross

@jeffbcross
Copy link
Contributor

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

@vicb
Copy link
Contributor Author

vicb commented Sep 8, 2015

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

@jeffbcross
Copy link
Contributor

Haha @vicb no worries

@vicb vicb deleted the 0831-lifecycle branch September 29, 2015 23:38
@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.
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.

None yet
8 participants
@vicb @kegluneq @keertip @mary-poppins @yjbanov @jeffbcross @mhevery @googlebot