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

fix: fails to check monorepo with non-published packages #89

Merged
merged 2 commits into from
May 29, 2019

Conversation

splix
Copy link
Contributor

@splix splix commented May 21, 2019

When you have a monorepo with some of the packages depends on other packages from same project it still tries to download package.json from npm central repo.

It becomes a problem when you have packages that were not published yet, at this case it always fails because it cannot find such package, though it exists locally and is a part of the project. For example if you use it within CI you it will always fail for commits that with new version of a package, unless you publish it before commit.

This PR suggest to skip verification of a dependency when it is another packages in same local project. The package itself will be checked as usual.

@DominicKramer
Copy link

Thanks for letting us know about the issue and opening this PR. We really appreciate it. We'll add some tests for your changes.

I also want to double check your changes for the case when the license checker runs on project A that depends on local project B (that has an invalid license). I think the check on A should still fail the license check, but it looks like B would be ignored in your implementation. I'll double check this to see if that is the case.

@splix
Copy link
Contributor Author

splix commented May 21, 2019

It will be ignores as a dependency verification, but as it's a part of current project it should be checked individually on it's own step.

As I see how it worked:

  1. Check package A
  2. Check package B as a dependency of A
  3. Check package B

My PR eliminates step 2 (because package B may be not published yet and should not be checked against npmjs), but actual check at step 3 should still happen

@ofrobots ofrobots mentioned this pull request May 29, 2019
@DominicKramer
Copy link

@splix that makes sense. I've added a test.

@ofrobots ofrobots merged commit 96a7279 into google:master May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants