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

Updated Angular from 7.2.5 to 8.0 using ng update #5431 #10961

Merged
merged 9 commits into from
Jul 2, 2019
Merged

Updated Angular from 7.2.5 to 8.0 using ng update #5431 #10961

merged 9 commits into from
Jul 2, 2019

Conversation

isaacrlevin
Copy link
Contributor

Summary of the changes

  • Updated Angular from 7.2.5 to 8.0 using ng update
  • Most diffs are related to optimizations by the upgrade

Addresses #5431

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 6, 2019
@ryanbrandenburg
Copy link
Contributor

Don't forget to add ClientApp/browserslist to the list of baseline files. This one is going to require closer review since it involves a fairly significant change, so it might take me a bit longer to get to.

@isaacrlevin
Copy link
Contributor Author

@ryanbrandenburg sounds good, added baseline

Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Couple of comments, nothing huge, mostly just formatting suggestions.

Over-all this looks good, I need to poke around at a running version to make sure everything still works, compare it to a default angular 8 app (we try to keep them similar) and get approval for the new version, but I suspect this is in a good place.

@@ -1,24 +1,44 @@
<header>
<nav class='navbar navbar-expand-sm navbar-toggleable-sm navbar-light bg-white border-bottom box-shadow mb-3'>
<nav
Copy link
Contributor

Choose a reason for hiding this comment

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

I was originally going to say "one tag, one line", but I checked out some style guides and it seems that keeping things to <80 characters/line is widely agreed on. Given that I'm OK with this.

<li class="nav-item" [routerLinkActive]='["link-active"]'>
<a class="nav-link text-dark" [routerLink]='["/counter"]'>Counter</a>
<li class="nav-item" [routerLinkActive]="['link-active']">
<a class="nav-link text-dark" [routerLink]="['/counter']"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say this looks better as

<a class="nav-link text-dark" [routerLink]="['/counter']">
    Counter
</a>

<a class="nav-link text-dark" [routerLink]='["/fetch-data"]'>Fetch data</a>
<li class="nav-item" [routerLinkActive]="['link-active']">
<a class="nav-link text-dark" [routerLink]="['/fetch-data']"
>Fetch data</a
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

<title>Company.WebApplication1</title>
<base href="/">
<head>
<meta charset="utf-8" />
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ryanbrandenburg
Copy link
Contributor

The project already had a browserslist int the ClientApp\src directory, we should remove that when we add the new one.

@ryanbrandenburg
Copy link
Contributor

@isaac2004 it looks like your baseline update commit didn't have a diff? Tests still fail due to baselines.

@isaacrlevin
Copy link
Contributor Author

@ryanbrandenburg super bizarre here, look at the change in baseline

image

That is where the browserslist file is, yet in the CI, it is looking at the old location, thoughts?
image

@ryanbrandenburg
Copy link
Contributor

@isaac2004 that means that there's an extra browserslist file in the src directory (it existed before your PR). Delete ClientApp/src/browserslist and those tests should pass.

@ryanbrandenburg
Copy link
Contributor

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com.

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2181
https://github.com/aspnet/AspNetCore-Internal/issues/2614
https://github.com/aspnet/AspNetCore-Internal/issues/2465

@isaacrlevin
Copy link
Contributor Author

@ryanbrandenburg helix failed but all else passed, let me know if you have any questions.

@ryanbrandenburg
Copy link
Contributor

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com.

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2489
https://github.com/aspnet/AspNetCore-Internal/issues/2648
https://github.com/aspnet/AspNetCore-Internal/issues/1979

@ryanbrandenburg ryanbrandenburg requested a review from danroth27 July 1, 2019 17:02
@isaacrlevin
Copy link
Contributor Author

@ryanbrandenburg can you look at this CI failure?

@ryanbrandenburg
Copy link
Contributor

@isaac2004 looks like you have some peer dependencies missing/of the wrong version:

npm WARN @angular/core@8.0.0 requires a peer of zone.js@~0.9.1 but none is installed. You must install peer dependencies yourself.\r\n
npm WARN @nguniversal/module-map-ngfactory-loader@7.1.0 requires a peer of @angular/common@^7.1.4 but none is installed. You must install peer dependencies yourself.\r\n
 npm WARN @nguniversal/module-map-ngfactory-loader@7.1.0 requires a peer of @angular/core@^7.1.4 but none is installed. You must install peer dependencies yourself.\r\n
 npm WARN @nguniversal/module-map-ngfactory-loader@7.1.0 requires a peer of @angular/platform-server@^7.1.4 but none is installed. You must install peer dependencies yourself.\r\n

@ryanbrandenburg
Copy link
Contributor

@isaac2004 seems we've also run into merge conflicts from one of my PR's that updates versions. Select the angular 8 versions for conflicts in package.json and rather than trying to resolve the package-lock.json file by hand just delete it and run npm install in that directory, then add the file back.

@ryanbrandenburg ryanbrandenburg merged commit c56fb80 into dotnet:master Jul 2, 2019
@isaacrlevin isaacrlevin deleted the 5431-work branch July 8, 2019 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants