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

Add 'reject-cycles' lerna option #5239

Merged
merged 1 commit into from
May 27, 2019
Merged

Add 'reject-cycles' lerna option #5239

merged 1 commit into from
May 27, 2019

Conversation

evidolob
Copy link
Contributor

@evidolob evidolob commented May 24, 2019

Now we have cyclic dependency problem
@theia/plugin-ext, @theia/scm depends on each other, and lerna provides this warning message:
Encountered a cycle in the dependency graph.This may cause instability! Packages in cycle are: "@theia/git", "@theia/plugin-ext-vscode", "@theia/plugin-ext", "@theia/scm", "@theia/example-browser", "@theia/example-electron"

This pull add --reject-cycles lerna option to fail build if any cyclic dependency exists

@evidolob evidolob added the quality issues related to code and application quality label May 24, 2019
@evidolob evidolob requested a review from kittaakos May 24, 2019 07:35
@evidolob evidolob self-assigned this May 24, 2019
@evidolob evidolob requested a review from akosyakov May 24, 2019 07:38
@kittaakos
Copy link
Contributor

@evidolob, perhaps you forgot to commit the actual fix. Is this in a WIP state?

@evidolob
Copy link
Contributor Author

No, I think @vinokurig is working on providing fix. I just wont to make sure that problem with cyclic dependencies never appears again.

@kittaakos
Copy link
Contributor

I just wont to make sure that problem with cyclic dependencies never appears again.

I think we can all agree, we won't merge your PR without the fix :)

@evidolob
Copy link
Contributor Author

Yes, of course

@vinokurig
Copy link
Contributor

@evidolob @kittaakos I am working on it right now

@vinokurig
Copy link
Contributor

@evidolob @kittaakos The Fix is ready, please review #5241

@kittaakos
Copy link
Contributor

Can you please rebase from the master, @evidolob? The builds should pass with the fix.

Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
@evidolob
Copy link
Contributor Author

Done

@kittaakos
Copy link
Contributor

Done

👍

Let's wait with the merge. See: #4279 (comment)

@vince-fugnitto
Copy link
Member

Will the PR also pick this cyclic dependency? #4895

@kittaakos
Copy link
Contributor

Will the PR also pick this cyclic dependency?

No. It checks npm package dependencies and cycles and not module dependencies.

@evidolob
Copy link
Contributor Author

@kittaakos Hi, it seems that #5241 are merged, can you review this pull?

@kittaakos
Copy link
Contributor

@evidolob, AFAIK, @akosyakov will go through the SCM-related changes manually and will check what functionality and commits did we lose. I'd leave the decision up to him.

@evidolob
Copy link
Contributor Author

@kittaakos OK, thx. @akosyakov Any objections to merge this pull?

@akosyakov
Copy link
Member

Could we enable it lerna.json instead, i.e. rejectCycles: true? that it works always

@evidolob
Copy link
Contributor Author

I try to add it in lerna.json first, but lerna wasn't work. I found reject-cycles parameter there https://github.com/lerna/lerna/tree/master/core/global-options#--reject-cycles It seems that it's only command line option.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

ok, thx

@evidolob evidolob merged commit e2c1993 into master May 27, 2019
@evidolob evidolob deleted the add-reject-cycles branch May 27, 2019 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality issues related to code and application quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants