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

Implement ng2 basic lifecycle hooks #119

Closed
wants to merge 1 commit into from
Closed

Implement ng2 basic lifecycle hooks #119

wants to merge 1 commit into from

Conversation

DcsMarcRemolt
Copy link

As the old pr #41 is back from the es6 days - here is my second try as we really need these callbacks in our project and I finally want to just npm install this lib.

@timkindberg
Copy link
Contributor

Thanks.

I think ngOnViewInit and ngOnDestroy are both in the right place, but I wonder about ngOnInit. Per this doc: https://angular.io/docs/ts/latest/api/core/OnInit-interface.html, it says:

Implement this interface to execute custom initialization logic after your directive's data-bound properties have been initialized.

But due to bug #81, I'm not 100% sure that data-bound properties (aka Inputs) will be ready by the time this hook is called. It might. I would need you to check in both TypeScript and Babel compilers.

So to get this merged:

  1. Check that data-bound properties are indeed initialized in both babel and typescript in the ngOnInit hook
  2. Write tests
  3. Write docs in the API.md
  4. Change commit message to match existing commits

Otherwise I do plan to work on this story when I get back into things. So maybe another month tops if you leave it to us.

@timkindberg
Copy link
Contributor

related to #72

@timkindberg
Copy link
Contributor

I think I'm going to pick this up. I'll certainly use what you've got here, but I'll take care to add the other required stuff (tests, docs) as well as maybe do onChange as well.

@timkindberg
Copy link
Contributor

Added those three lifecycle hooks. Only code that I had to modify of yours was the After View Init.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants