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

Update outdated npm dependencies #15318

Merged
merged 5 commits into from
Oct 30, 2019
Merged

Conversation

mkArtakMSFT
Copy link
Member

@mkArtakMSFT mkArtakMSFT commented Oct 23, 2019

Description

The Angular project template are currently producing an error during project start.

Customer Impact

Starting new Angular SPA projects produces a red lined error in console, while still being runnable.

Regression?

No. This is not something we have control over so our approach here is always reactive.

Risk

Low. The change has been validated manually and the apps can be run successfully.

Addresses https://github.com/aspnet/AspNetCore-ManualTests/issues/43

@mkArtakMSFT mkArtakMSFT added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Oct 23, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.1.0-preview2 milestone Oct 23, 2019
@mkArtakMSFT mkArtakMSFT requested a review from pranavkm October 23, 2019 14:46
@mkArtakMSFT mkArtakMSFT changed the base branch from master to release/3.1 October 23, 2019 14:47
@javiercn
Copy link
Member

Is there no requirement to update the package.json files themselves?

@mkArtakMSFT
Copy link
Member Author

@javiercn I just run 'npm update' and 'npm audit fix' and it does the right thing. So seems there were no required changes to the package.json

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.

This seems fine, the packages which were removed from package.json seem to still be brought into package.lock.json transitively, so we didn't really lose anything.

@mkArtakMSFT
Copy link
Member Author

@ryanbrandenburg, @javiercn here what I've done.

  • I first tried to simply do npm update / install but it seems there is some dependency version which got updated and it started to bring in one after another a lot of dependencies.
  • In the end, I just upgraded everything to the most recent available major version (refering to npm packages here)
  • I've validated all projects (Angular, React, ReactRedux) by creating a new project, applying these changes to them and running. I was able to successfully validate all the pages.
  • I've also run npm run lint on the ClientApp folder and it is running successfully for all these three projects

One thing I just now thought about is that I haven't validated anything with auth enabled. So maybe there are things to be done there. Not sure that can cause a problem or not. Please suggest.

@javiercn
Copy link
Member

@mkArtakMSFT The tests are not uber-complete but they test enough stuff I think. They essentially click fetch data and go through the login process and validate that the user authorizes successfully.

For a simple dependency update there should be not much issue.

@mkArtakMSFT
Copy link
Member Author

@javiercn, @ryanbrandenburg I am now slowly concluding that this is more like a test issue.
As part of my last verification I've downloaded the generated packages as part of the above failed builds, and created a new identity-enabled react project and manually run through it. Everything works!

@ryanbrandenburg
Copy link
Contributor

@mkArtak looks like you needed some using's.

@mkArtakMSFT
Copy link
Member Author

Talked to @ryanbrandenburg regarding this and from his experience it may be that the test looking for the fetch-data doesn't wait enough. So I'm validating that now with my last commit if that turns out to be the case, will simply increase the timeout.

@javiercn
Copy link
Member

@mkArtakMSFT I don't think that's the case. By default tests now wait 2 minutes before the first assertion fails, and 30 seconds for the next assertion. If you don't have multiple failing tests, it's unlikely you are hitting a timeout.

@ryanbrandenburg
Copy link
Contributor

Yeah, given that browser.Contains already does retries I suspect @javiercn is right.

@javiercn
Copy link
Member

@mkArtakMSFT lets look at this together tomorrow

@mkArtakMSFT mkArtakMSFT added the Servicing-consider Shiproom approval is required for the issue label Oct 29, 2019
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/npmUpdate3 branch from eb953c0 to bc919d4 Compare October 29, 2019 15:45
@mkArtakMSFT
Copy link
Member Author

I've ended up reverting the React related changes to avoid the headache. We'll deal with it separately. for now making only Angular -targeted fix.

@mkArtakMSFT mkArtakMSFT changed the base branch from release/3.1 to release/3.1-preview2 October 29, 2019 16:47
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/npmUpdate3 branch from bc919d4 to a31f7f1 Compare October 29, 2019 16:51
@mkArtakMSFT mkArtakMSFT removed the Servicing-consider Shiproom approval is required for the issue label Oct 29, 2019
@mkArtakMSFT
Copy link
Member Author

After re-validating it seems the issue is not that bad. The app still works, while producing the error.

@mkArtakMSFT mkArtakMSFT changed the base branch from release/3.1-preview2 to release/3.1 October 29, 2019 17:19
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/npmUpdate3 branch from a31f7f1 to 2b2e773 Compare October 29, 2019 17:20
@mkArtak
Copy link
Contributor

mkArtak commented Oct 30, 2019

I think I found the reason for the last couple of failures. It's a react issue being tracked svg/svgo#1174

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Merge this ASAP :)

@mkArtakMSFT mkArtakMSFT merged commit 8e3075d into release/3.1 Oct 30, 2019
@mkArtakMSFT mkArtakMSFT deleted the mkArtakMSFT/npmUpdate3 branch October 30, 2019 14:05
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

5 participants