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

feat: better error when package is not found #5213

Merged
merged 1 commit into from
Jan 29, 2018

Conversation

GAumala
Copy link
Contributor

@GAumala GAumala commented Jan 13, 2018

Summary

When a package is not found during installation, show which package is requiring it (if any) in the
error message. If there is no parent request, throw the same error as before.

Add new localized string 'requiredPackageNotFoundRegistry' which is the same 'packageNotFoundRegistry', but includes the parent package pattern.
Add isMessageError boolean value to MessageError class to distinguish
between errors thrown by yarn from errors thrown by yarn dependencies.

Test plan

Use a workspace with at least 2 packages foo and bar. bar depends on foo. If you manually bump the package version of foo, running the install command will fail because the npm registry doesn't have the latest foo version that was just created.

Error shown (Before)

screenshot from 2018-01-13 17-42-27

Error shown (After)

screenshot from 2018-01-13 17-42-46

To implement this I decided to put a try catch statement in PackageRequest's findVersionOnRegistry method. I think the error is actually thrown by the request module but I decided to catch the error here because I still have access to the reference to the parent request. I didn't want to hide any MessageError errors that may be thrown in the underlying function so I added a isMessageError attribute to that class so that I can identify such errors.

Please let me know if there is a better way to implement this feature.

Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Nice work, it looks great, thanks a lot! I'll merge it as soon as you update the PR to use instanceof 👍

src/errors.js Outdated
@@ -4,9 +4,11 @@ export class MessageError extends Error {
constructor(msg: string, code?: string) {
super(msg);
this.code = code;
this.isMessageError = true;
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 use an instanceof check instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was thinking about using instanceof, but what if subclasses of MessageError are thrown? Should I check for all of them or only MessageError?

Copy link
Member

Choose a reason for hiding this comment

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

Only MessageError is fine. If they're subclass, the instanceof will work as expected and will return true, even if they are from a derived class.

When a package is not found during installation, show which package is
requiring it (if any) in the error message. If there is no parent request,
throw the same error as before.
Add new localized string 'requiredPackageNotFoundRegistry' which is the
same 'packageNotFoundRegistry', but includes the parent package pattern.
@buildsize
Copy link

buildsize bot commented Jan 15, 2018

This change will increase the build size from 10.4 MB to 10.4 MB, an increase of 2.06 KB (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 900.89 KB 900.94 KB 52 bytes (0%)
yarn-[version].js 3.92 MB 3.92 MB 578 bytes (0%)
yarn-legacy-[version].js 4.07 MB 4.07 MB 573 bytes (0%)
yarn-v[version].tar.gz 905.9 KB 906.66 KB 774 bytes (0%)
yarn_[version]all.deb 669.58 KB 669.71 KB 134 bytes (0%)

@arcanis arcanis merged commit 18ea344 into yarnpkg:master Jan 29, 2018
@arcanis
Copy link
Member

arcanis commented Jan 29, 2018

Thanks!

agoldis added a commit to agoldis/yarn that referenced this pull request Feb 2, 2018
…readdir_files

* upstream/master: (34 commits)
  feat(upgrade, add): Separately log added/upgraded dependencies (yarnpkg#5227)
  feat(publish): Publish command uses publishConfig.access in package.json (yarnpkg#5290)
  fix(CLI): Use process exit instead of exitCode for node < 4 (yarnpkg#5291)
  feat(cli): error on missing workspace directory (yarnpkg#5206) (yarnpkg#5222)
  feat: better error when package is not found (yarnpkg#5213)
  Allow scoped package as alias source (yarnpkg#5229)
  fix(cli): Use correct directory for upgrade-interactive (yarnpkg#5272)
  nohoist baseline implementation (yarnpkg#4979)
  1.4.1
  1.4.0
  Show current version, when new version is not supplied on "yarn publish" (yarnpkg#4947)
  fix(install): use node-gyp from homebrew npm (yarnpkg#4994)
  Fix transient symlinks overriding direct ones v2 (yarnpkg#5016)
  fix(auth): Fixes authentication conditions and logic with registries (yarnpkg#5216)
  chore(package): move devDeps to appropriate place (yarnpkg#5166)
  fix(resolution) Eliminate "missing peerDep" warning when dep exists at root level. (yarnpkg#5088)
  fix(cli): improve guessing of package names that contain a dot (yarnpkg#5102) (yarnpkg#5135)
  feat(cli): include notice with license when generating disclaimer (yarnpkg#5072) (yarnpkg#5111)
  feat(cli): group by license in licenses list (yarnpkg#5074) (yarnpkg#5110)
  feat(cli): improve error message when file resolver can't find file (yarnpkg#5134) (yarnpkg#5145)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants