Skip to content

fix(@schematics/update): ignore npm 404 error #13167

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
Merged
merged 1 commit into from
Dec 20, 2018
Merged

fix(@schematics/update): ignore npm 404 error #13167

merged 1 commit into from
Dec 20, 2018

Conversation

tiaguinho
Copy link
Contributor

This PR will fix the issue #10614

@tiaguinho
Copy link
Contributor Author

What do you guys think?
cc: @hansl @clydin

alan-agius4
alan-agius4 requested changes Dec 11, 2018
View reviewed changes
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM, though can you kindly add the fixes footer in the commit message?

Ex

fix(@schematics/update): ignore npm 404 error

Fixes 10614

@tiaguinho
Copy link
Contributor Author

@alan-agius4 just did it!

clydin
clydin requested changes Dec 11, 2018
View reviewed changes
Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

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

This will have the unfortunately side effect of hiding legitimate errors as well.
Can you change this to catch errors at the getNpmPackageJson call site rather than within the function itself and log the error so that the user has knowledge of the issue.

Ideally, the getNpmPackageJson function should not be called if the package specifier does not point to a registry.

@tiaguinho
Copy link
Contributor Author

@clydin we can use the workspace configuration to search local packages and filter _getAllDependencies.
This way, only npm packages will be pass to getNpmPackageJson.

@tiaguinho tiaguinho force-pushed the issue/10614 branch 4 times, most recently from 13e6cc3 to ec89e3d Compare December 12, 2018 17:37
@tiaguinho
Copy link
Contributor Author

Changed!

cc: @alan-agius4 @clydin

clydin
clydin requested changes Dec 19, 2018
View reviewed changes
packages/schematics/update/update/npm.ts
catchError(err => {
logger.warn(err);

return EMPTY;
Copy link
Member

Choose a reason for hiding this comment

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

use empty() instead of EMPTY.

Copy link
Contributor

Choose a reason for hiding this comment

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

Disregard, EMPTY is fine (see https://rxjs.dev/api/index/const/EMPTY)

packages/schematics/update/update/npm.ts Outdated
const response = from<NpmRepositoryPackageJson>(resultPromise).pipe(
shareReplay(),
catchError(err => {
logger.warn(err);
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to logger.warn(err.message || err)? The user doesn't need the full stack trace.

@tiaguinho tiaguinho force-pushed the issue/10614 branch 2 times, most recently from 7bc4ea5 to d2486a6 Compare December 19, 2018 20:07
@tiaguinho
Copy link
Contributor Author

Hi @clydin just made the changes you asked.
If I miss something, please let me know.

Thanks!

hansl
hansl suggested changes Dec 19, 2018
View reviewed changes
packages/schematics/update/update/npm.ts Outdated
catchError(err => {
logger.warn(err.message || err);

return empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, please use EMPTY (see https://rxjs.dev/api/index/const/EMPTY).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hahaha, no problem.

@hansl
Copy link
Contributor

hansl commented Dec 19, 2018

Additionally, could you add a // TODO: find some way to test this? Testing npm requests is hard, but we should have proper integration tests with this (in some future).

@tiaguinho
fix(@schematics/update): ignore npm 404 error
fe1a7e6 
Fixes 10614

style(@schematics/update): lint error

fix(@schematics/update): remove full stack trace

style(@schematics/update): organize import order

refactor(@schematics/update): use empty constant
@mgechev mgechev added target: major This PR is targeted for the next major release target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Dec 20, 2018
@hansl
Copy link
Contributor

hansl commented Dec 20, 2018

Thanks @tiaguinho ! This looks good to go in. I'll try to get @alan-agius4 to approve.

@alan-agius4 alan-agius4 removed the target: patch This PR is targeted for the next patch release label Dec 20, 2018
@alan-agius4 alan-agius4 added the target: patch This PR is targeted for the next patch release label Dec 20, 2018
@hansl hansl merged commit 2bf3765 into angular:master Dec 20, 2018
@daniloff200 daniloff200 mentioned this pull request Dec 22, 2018
Closed
@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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mgechev mgechev mgechev approved these changes

@hansl hansl hansl approved these changes

@alan-agius4 alan-agius4 alan-agius4 approved these changes

@clydin clydin clydin approved these changes

Assignees
No one assigned
Labels
target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tiaguinho @hansl @mgechev @alan-agius4 @clydin @googlebot