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

The mgit command(s) should ignore main package. #96

Closed
jodator opened this issue Jan 11, 2019 · 6 comments
Closed

The mgit command(s) should ignore main package. #96

jodator opened this issue Jan 11, 2019 · 6 comments

Comments

@jodator
Copy link
Contributor

jodator commented Jan 11, 2019

After debugging https://github.com/ckeditor/ckeditor5-paragraph/issues/39 and talks with @Reinmar we came to conclusion that we had a bug which npm+lerna somehow hidden from us.

The problem is when package being tested requires another package that requires the main package (it might be by a devDependencies).

In this case it was ckeditor5-paragraph that requires ckeditor5-heading in integration tests. The HeadingEditing requires Paragraph package. It should be sufficient to omit ckeidtor5-paragraph when running mgit bootstrap.

Right now I can see that we can:

  1. Use existing --ignore switch to simply ignore some package - it has one drawback that this has bug/feature that it only ignores immediate packages and does not ignore packages from --ignore if not ignored package require it
  2. Add a feature that it will always ignore current package - I can see that this should only affect bootstrap as other commands kinda depends on what's inside packages and we only want to prevent current package to be imported into packages/* if required by other plugins.

cc @Reinmar, @pomek

@jodator
Copy link
Contributor Author

jodator commented Jan 11, 2019

ps.: now it's sync command :D

@Reinmar
Copy link
Member

Reinmar commented Jan 11, 2019

The second option seems logically correct. If you run mgit bootstrap from package-foo, then if package-foo is its transient dependency, there's no point in cloning it.

@pomek
Copy link
Member

pomek commented Jan 11, 2019

I guess the problem is in the script that creates the mgit.json file. It does not care about filtering packages. So if ckeditor5-heading requires ckeditor5-paragraph (which is already the root), it should not be added to the config file.

See: https://github.com/ckeditor/ckeditor5-dev/blob/25035552eda1fd22e27fbf4b53cf79536b3f00a4/packages/ckeditor5-dev-tests/lib/bin/createmgitjsoncontent.js

From the Mgit perspective, it's nonsense to add a package which is a root to its packages. I mean:

|- ckeditor5-paragraph
|-- |- packages
|-- |-- |- ckeditor5-paragraph # it should not be here

@jodator
Copy link
Contributor Author

jodator commented Jan 11, 2019

I guess the problem is in the script that creates the mgit.json file.

Thanks for the tip. I'll check that.

@pomek
Copy link
Member

pomek commented Jan 11, 2019

I was wrong. You are right guys. Mgit shouldn't install the root package as its package.

@jodator
Copy link
Contributor Author

jodator commented Jan 11, 2019

It looks like the fix is quite simple. Just ignore the packages that name equals to main package name in enque() call of index.js. Unfortunately this file has no tests.

@pomek pomek closed this as completed in #97 Jan 11, 2019
pomek added a commit that referenced this issue Jan 11, 2019
Fix: The master repository package should not be loaded to packages dir. Closes #96.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants