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

[DO NOT MERGE IN 2.x - 4.x only] refactor(hooks): change abstract classes for interfaces #12324

Merged
merged 1 commit into from
Feb 24, 2017
Merged

[DO NOT MERGE IN 2.x - 4.x only] refactor(hooks): change abstract classes for interfaces #12324

merged 1 commit into from
Feb 24, 2017

Conversation

DzmitryShylovich
Copy link
Contributor

Fixes #10083

@vicb
Copy link
Contributor

vicb commented Oct 15, 2016

Thanks for the PR:

@vicb vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews state: blocked action: review The PR is still awaiting reviews from at least one requested reviewer breaking changes area: core Issues related to the framework runtime and removed state: blocked labels Oct 15, 2016
@DzmitryShylovich DzmitryShylovich force-pushed the feature/10083 branch 2 times, most recently from 1126bfc to 93c413b Compare December 28, 2016 20:11
@DzmitryShylovich
Copy link
Contributor Author

@vicb We can probably merge this into 4.0 branch

@vicb
Copy link
Contributor

vicb commented Dec 29, 2016

Yes once:

  • CI is green,
  • Commit headline is refactor(...): change abstract classes for interfaces,
  • Commit message state that this is a breaking change.

@vicb vicb changed the title refactor(docs): update public api [DO NOT MERGE IN 2.x - 4.x only] refactor(hooks): change abstract classes for interfaces Dec 29, 2016
@DzmitryShylovich
Copy link
Contributor Author

@vicb done

@vicb vicb added action: merge The PR is ready for merge by the caretaker pr_state: LGTM and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 29, 2016
@vicb
Copy link
Contributor

vicb commented Dec 29, 2016

thanks !

@IgorMinar
Copy link
Contributor

I was about to merge this in but then realized that the breaking change is not properly documented (in terms of impact on the end users) and upon further digging in I realized that this is actually a bigger breaking change than what it look like. I'd like to have us take one more look at the impact of this change before we merge it in.

I describe my concerns in this typescript snippet: https://goo.gl/sW8qLW

@DzmitryShylovich
Copy link
Contributor Author

that the breaking change is not properly documented

I think u are underestimate your users :)
Just would need to add a note in the changelog that it can possibly break some code and it would be enough.
All examples on angular.io use implements. It doesn't mention abstract classes at all https://angular.io/docs/ts/latest/guide/lifecycle-hooks.html#!#interface-optional
We can call it an impl detail

in case someone created a popular blog post, tutorial, seed

so we shouldn't change code because of a blog post? I think it's a dead end

Plus in 2.3.1 angular added support for inheritance and using extends for lifycycle hooks u will no longer abler to extend anything else.

@vicb
Copy link
Contributor

vicb commented Jan 4, 2017

  • I agree that the commit message should not discuss the impl but the impact on the user (ie, use implements instead of extends as discussed yesterday) @DzmitryShylovich can you update ?
  • User code could easily be upgraded by a tool here - I guess we will have many other changes requiring a this tool in 4.x

@mhevery
Copy link
Contributor

mhevery commented Jan 4, 2017

I think it is unlikely that people today are extending OnInit rather than implementing. I think this is fine for v4, but needs better documentation as pointed out.

@IgorMinar
Copy link
Contributor

I spoke with @mhevery and @juleskremer and we agreed that Angular DevRel is going to review this, estimate impact and weigh in on this before we merge it in.

The problem is that our promise to the community was that no changes in Angular 4 would break existing components. We don't know if any existing components that people depend on rely on "extends" rather than "implements" and if there are any that do, then people won't be able to use them if they are 3rd party components that they can't change. We promised to make the upgrade to A4 smooth and this change has a potential to make it not smooth, so we need to be cautions.

DevRel will review this, and suggest the path forward for this change.

Just to be clear, I think this is a good change, we just need to be careful about how we roll it out so that we don't break the Angular ecosystem and with that our promises to the community.

@IgorMinar IgorMinar removed the action: merge The PR is ready for merge by the caretaker label Jan 5, 2017
@robwormald
Copy link
Contributor

is this something that tslint or codeylzer could pick up?

@IgorMinar
Copy link
Contributor

@robwormald it could. it might be a good idea to roll that out now (specific to our hooks only) so that we start warning people about this now. But we still don't know how many projects are being covered by lint/codelyzer and how many of them will use the updated version before A4.

@DzmitryShylovich
Copy link
Contributor Author

DzmitryShylovich commented Jan 5, 2017 •
edited
Loading

what's so special with lifecycle hooks? For example, this #13785 pr will break all apps because every decent app uses OpaqueTokens when this pr will probably break <1% users. Just interesting.

@IgorMinar
Copy link
Contributor

@DzmitryShylovich I pushed back on that one as well. We need to really understand the impact of these kinds of changes before merging them in.

@DzmitryShylovich
Copy link
Contributor Author

It will also decrease file size.

@IgorMinar
Copy link
Contributor

@DzmitryShylovich can you please rebase this one? If things are looking good with the view engine, we'll attempt to land this PR.

refactor(core): change abstract classes for interfaces
3237d43 
BREAKING CHANGE: Because all lifecycle hooks are now interfaces
the code that uses 'extends' keyword will no longer compile.

To migrate the code follow the example below:

Before:

@component()
class SomeComponent extends OnInit {}

After:

@component()
class SomeComponent implements OnInit {}

Closes angular#10083
@DzmitryShylovich
Copy link
Contributor Author

@IgorMinar done

@IgorMinar IgorMinar merged commit ee747f7 into angular:master Feb 24, 2017
@IgorMinar
Copy link
Contributor

thanks @DzmitryShylovich

@DzmitryShylovich DzmitryShylovich deleted the feature/10083 branch February 24, 2017 07:33
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
@DzmitryShylovich
refactor(core): change abstract classes for interfaces (angular#12324)
3293107 
BREAKING CHANGE: Because all lifecycle hooks are now interfaces
the code that uses 'extends' keyword will no longer compile.

To migrate the code follow the example below:

Before:
```
@component()
class SomeComponent extends OnInit {}
```
After:
```
@component()
class SomeComponent implements OnInit {}
```

we don't expect anyone to be affected by this change.

Closes angular#10083
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@DzmitryShylovich @juleskremer
refactor(core): change abstract classes for interfaces (angular#12324)
25e7525 
BREAKING CHANGE: Because all lifecycle hooks are now interfaces
the code that uses 'extends' keyword will no longer compile.

To migrate the code follow the example below:

Before:
```
@component()
class SomeComponent extends OnInit {}
```
After:
```
@component()
class SomeComponent implements OnInit {}
```

we don't expect anyone to be affected by this change.

Closes angular#10083
@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 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
area: core Issues related to the framework runtime breaking changes cla: yes state: blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lifecycle interfaces became abstract classes
6 participants
@DzmitryShylovich @vicb @IgorMinar @mhevery @robwormald @googlebot