-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Added typescript fix for problem with specs created by "dotnet new angular --auth" #18295
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
Added typescript fix for problem with specs created by "dotnet new angular --auth" #18295
Conversation
Individual" generated code.
|
Thanks for your PR, @benday. Please target |
|
@mkArtakMSFT -- FYI, I changed the target branch from 3.0 to master. Hopefully this should keep the changes more readable. |
ryanbrandenburg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this doesn't seem to fix all the tests when merged with the master branch.
...ntent/Angular-CSharp/ClientApp/src/api-authorization/login-menu/login-menu.component.spec.ts
Outdated
Show resolved
Hide resolved
refresh from master
Update login-menu.component.spec.ts to remove trailing space
benday
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed trailing spaces per PR comments
Individual" generated code.
Update login-menu.component.spec.ts to remove trailing space
…com/benday/aspnetcore into webapi-angular-template-issue-18121
|
I'm still seeing failures such as: when I attempt to run these tests. Are you not seeing those when you run it locally? If not let me know what steps your taking to test so I can follow them. |
|
Yah. I'm seeing that, too. It was working and now it isn't. BTW, the "dotnet new angular" template targets Angular 8. I've got this working with Angular 9. Would you like me to add my Angular 9 template updates to this PR along with the security unit test changes? |
Moving to Angular 9 is a bigger thing (if not for the code, then from the book-keeping we do about dependencies on our end). I would prefer to keep the two separate. |
|
gotcha. should I submit a PR for Angular 9 or would you like to push that to a later date entirely? |
|
The issue for upgrading to Angular 9 is here. Looks like we had previously delayed due to problems people were finding with upgrading. @danroth27 is driving some other work related to our SPA templates and could maybe opine if you asked on that issue. |
Hi @ryanbrandenburg -- I'm just getting back to this and i can only repro those errors if I add "--framework netcoreapp3.1" to the dotnet new command. I'm stumped. Hopefully you can help. Here's my process:
For example, this creates code that works fine: This creates code that gives all the exceptions you mentioned: I did a compare between the code that's generated off of those two commands and the "--framework netcoreapp3.1" doesn't have any of the changes that I've submitted. I suspect that specifying Any help would be greatly appreciated. Thanks, |
|
@HaoK can you please take this PR over from @ryanbrandenburg and work with @benday to get this merged? Thanks! |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@benday looks like all the CI checks are passing now, are you still seeing any failures or is this good to go from your perspective? |
|
@HaoK -- Looks good to me. No changes from my end. I'd say it's good to go. Lemme know if you need anything. |
Dismissing review since checks are green now
|
Thanks for the PR @benday |
Summary of the changes (Less than 80 chars)
Addresses #18121