Skip to content
This repository was archived by the owner on Sep 9, 2024. It is now read-only.

Conversation

@bob-watson
Copy link
Contributor

Updates the text to include the guide to update from v14 to v15.
Also includes minor UI text edits.

Updated the level to an enum for better readability and initiated the
update instructions to v15.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this one third? What is the reason you added it to the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ordered them this way to appear as a checklist in which, if all looked good, you'd update your application.
Because you can't un-update an application, this seemed the safest order to prevent:

  • read
  • update
  • read further and facepalm

Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend moving ng update first because some of these changes are not available in v14 which will cause errors when folks make the changes we suggest here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Thanks!

Copy link
Member

@mgechev mgechev Nov 3, 2022

Choose a reason for hiding this comment

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

I'd suggest updating to:

Suggested change
{ possibleIn: 1500, necessaryAsOf: 1500, level: ApplicationComplexity.Basic, step: 'v15 keyframe', action: 'In v15, `@keyframes` names are prefixed with the component\'s <emphasis>scope name</emphasis>. <a href="https://angular.io/guide/update-to-version-15#v15-bc-03" alt="Link to more information about this change">Read further</a>' },
{ possibleIn: 1500, necessaryAsOf: 1500, level: ApplicationComplexity.Basic, step: 'v15 keyframe', action: 'The Angular compiler now prefixes `@keyframes` in CSS with components' scope. This means that TypeScript code that relies keyframe names now will no longer work. Recommended update is to define keyframes programmatically, use global stylesheets, or change component's view encapsulation. <a href="https://angular.io/guide/update-to-version-15#v15-bc-03" alt="Link to more information about this change">Read further</a>' },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in current commit

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ possibleIn: 1500, necessaryAsOf: 1500, level: ApplicationComplexity.Basic, step: '', action: 'In v15, when a class inherits its constructor from a base class, the compiler can report an error when that constructor cannot be used for dependency injection purposes. <a href="https://angular.io/guide/update-to-version-15#v15-bc-05" alt="Link to more information about this change">Read further</a>'},
{ possibleIn: 1500, necessaryAsOf: 1500, level: ApplicationComplexity.Medium, step: '', action: 'Make sure you use decorators in base classes when their child classes inherit constructors and use dependency injection. Base classes should be decorated with `@Injectable` or `@Directive` alternatively the compiler will throw an error. <a href="https://angular.io/guide/update-to-version-15#v15-bc-05" alt="Link to more information about this change">Read further</a>'},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for:
Base classes should be decorated with @Injectableor@directive alternatively the compiler will throw an error.
would this work?
Base classes should be decorated with either @Injectableor@directive or the compiler returns an error.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this sounds good. @Directive should be uppercase, just like @Injectable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

@mgechev mgechev left a comment

Choose a reason for hiding this comment

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

Let us tell developers what to do about a deprecation rather than repeat what we've changed.

Copy link
Member

Choose a reason for hiding this comment

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

We can recommend people to review their application to prevent visual breakages.

Copy link
Contributor Author

@bob-watson bob-watson Nov 8, 2022

Choose a reason for hiding this comment

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

Added in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'll move this to the top as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'll change this from basic to medium.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'll change this from basic to medium.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'll change this from basic to medium.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'll change this from basic to advanced.

Can we tell people what they need to do and after share why they need to make a change?

Copy link
Contributor Author

@bob-watson bob-watson Nov 8, 2022

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

That's definitely advanced, not basic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

Can we say: "If your tests with RouterOutlet break, make sure you don't depend on the instantiation order of the corresponding component relative to change detection execution. As of v15 now..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

That's advanced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

Can we say what's the alternative of canParse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

@mgechev mgechev Nov 8, 2022

Choose a reason for hiding this comment

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

setDisabledState is medium/advanced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

Can we say what's the action item for developers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

Missing step name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say this is medium.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member

@mgechev mgechev left a comment

Choose a reason for hiding this comment

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

Left a few more comments.

Bob Watson and others added 2 commits November 8, 2022 13:33
Co-Authored-By: Minko Gechev <[email protected]>
Co-Authored-By: Minko Gechev <[email protected]>
@bob-watson
Copy link
Contributor Author

@mgechev I've updated this to address your feedback.

@bob-watson bob-watson marked this pull request as ready for review November 8, 2022 21:35
@bob-watson bob-watson marked this pull request as draft November 8, 2022 21:41
@bob-watson bob-watson marked this pull request as ready for review November 8, 2022 21:42
@mgechev mgechev merged commit 9a1bf81 into angular:main Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants