improved monorepo docs #2969

onursumer · 2020-01-03T22:37:25Z

Added tips for dependency management and package.json updates.

Checks

Has tests or has a separate issue that describes the types of test that should be created. If no test is included it should explicitly be mentioned in the PR why there is no test.
The commit log is comprehensible. It follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!

Any screenshots or GIFs?

If this is a new visual feature please add a before/after screenshot or gif
here with e.g. GifGrabber.

Notify reviewers

Read our Pull request merging
policy. It can help to figure out who worked on the
file before you. Please use git blame <filename> to determine that
and notify them either through slack or by assigning them as a reviewer on the PR

inodb · 2020-01-03T22:48:04Z

README.md

+
+Since now all the packages live under a single repository it might be confusing how to import different modules into the file you're working on.
+
+If you are working on `cbioportal-frontend` repository feel free to import any module from any package under `packages`. When importing from internal packages always use import statements like `import {something} from 'cbioportal-frontend-commons'`. Never do under any circumstances something like `import {something} from ../../packages/cbioportal-frontend-commons/src/lib/someLib` or `import {something} from ../../packages/react-mutation-mapper/src/component/someComponent`. Treat internal packages like any other npm package you would import.


might be good to have a circle ci check or github action for this at some point, could be as simple as

git grep -q "../packages/" src/** && exit 1

but maybe this is already caught by the build process itself?

inodb · 2020-01-03T22:49:40Z

README.md

+
+If you are working on `cbioportal-frontend` repository feel free to import any module from any package under `packages`. When importing from internal packages always use import statements like `import {something} from 'cbioportal-frontend-commons'`. Never do under any circumstances something like `import {something} from ../../packages/cbioportal-frontend-commons/src/lib/someLib` or `import {something} from ../../packages/react-mutation-mapper/src/component/someComponent`. Treat internal packages like any other npm package you would import.
+
+Avoid circular dependencies at all costs. For example, while it is okay to import a module from `cbioportal-frontend-commons` in `react-mutation-mapper`, there should not be any imports from `react-mutation-mapper` in `cbioportal-frontend-commons`. If you happen to need some component from from `react-mutation-mapper` in `cbioportal-frontend-commons`, consider moving that component into `cbioportal-frontend-commons` package.


does adding a circular dependency result in some error while building?

inodb · 2020-01-03T22:50:17Z

README.md

+
+### Updating existing packages
+
+Whenever you need to update code under packages, you should also consider updating the version number in the corresponding `package.json` as well as the dependencies of other packages depending on the package you updated. For example if you update the `cbioportal-frontend-commons` version from`0.1.1` to `0.1.2`, corresponding `cbioportal-frontend-commons` dependency in the `package.json` for `react-mutation-mapper` and `cbioportal-frontend` should also be updated to the new version.


is there a lerna or yarn workspace command that can help with this?

inodb · 2020-01-03T22:53:58Z

README.md

+### Updating existing packages
+
+Whenever you need to update code under packages, you should also consider updating the version number in the corresponding `package.json` as well as the dependencies of other packages depending on the package you updated. For example if you update the `cbioportal-frontend-commons` version from`0.1.1` to `0.1.2`, corresponding `cbioportal-frontend-commons` dependency in the `package.json` for `react-mutation-mapper` and `cbioportal-frontend` should also be updated to the new version.


kinda same here as with other comments. Should prolly be some ci or lerna/yarn build failure when trying to build something that refers to an older version of a package included in the monorepo? I guess in some cases this might be desired behavior but ideally we would avoid that

inodb

Thanks @onursumer ! This looks good to me

If necessary we can open some issues about the questions I raised. I think ideally most of these things would be taken care of by the build process, so developers are alerted early about these pitfalls. If that's not the case or the messages the build process spits out are too cryptic, we can add some CI checks

alisman · 2020-01-06T19:23:04Z

@Luke-Sikina have a look at the documentation for monorepo and let me know what you think

Luke-Sikina

Few tiny things, but it looks reasonable. Any comment prefixed Suggestion: is optional. As far as the updating of version numbers goes, is there a simple way to make sure we corrected the version numbers of all workspaces that depend on the one we changed?

Luke-Sikina · 2020-01-06T20:43:46Z

README.md

@@ -271,6 +271,31 @@ In order to add a new workspace, one should create a new directory under `packag



Line 268 typo: pakcages instead of packages

Luke-Sikina · 2020-01-06T20:48:47Z

README.md

@@ -271,6 +271,31 @@ In order to add a new workspace, one should create a new directory under `packag

 Suggested way to add a new dependency to an existing workspace is to run `yarn workspace <workspace name> add <package name>` instead of just `yarn add <package name>`. For example, run `yarn workspace cbioportal-frontend add lodash` instead of `yarn add lodash`. Similarly to remove a package run `yarn workspace <workspace name> remove <package name>`.

+### Tips for dependency management


Suggestion: add a subheading titled Adding a New Workspace before line 270. You already have the information, but it doesn't stand out.

Luke-Sikina · 2020-01-06T20:50:04Z

README.md

+The cBioPortal uses a monrepo. In a monorepo, multiple related packages are housed in single repository and published separately. E.g. the main portal project (cbioportal-frontend) lives in the same repository as the libraries which it uses, but which are also
+published for use by other projects.  Please abide by the following rules for working in a monorepo:   
+
+1. If you are working on `cbioportal-frontend` repository, import modules from packages using the package's alias:


Nice. I like these tips.

Luke-Sikina · 2020-01-06T20:52:34Z

README.md

+
+Remember that the packages are used by other projects and compatibility needs to be carefully managed. 
+
+Whenever you need to update code under packages, you should also consider updating the version number in the corresponding `package.json` as well as the dependencies of other packages depending on the package you updated. For example if you update the `cbioportal-frontend-commons` version from`0.1.1` to `0.1.2`, corresponding `cbioportal-frontend-commons` dependency in the `package.json` for `react-mutation-mapper` and `cbioportal-frontend` should also be updated to the new version.


from0.1.1
you're missing a space character.

onursumer requested review from alisman and inodb January 3, 2020 22:37

inodb reviewed Jan 3, 2020

View reviewed changes

inodb approved these changes Jan 3, 2020

View reviewed changes

alisman force-pushed the monorepo-docs-update branch from afe6f84 to a4859bb Compare January 6, 2020 16:16

jjgao temporarily deployed to cbioportal-f-monorepo-d-kibvex January 6, 2020 16:16 Inactive

inodb approved these changes Jan 6, 2020

View reviewed changes

alisman requested a review from Luke-Sikina January 6, 2020 19:22

Luke-Sikina approved these changes Jan 6, 2020

View reviewed changes

Improve monorepo documentation in ReadMe

4af7f48

alisman force-pushed the monorepo-docs-update branch from a4859bb to 4af7f48 Compare January 6, 2020 21:16

alisman merged commit f9316d7 into master Jan 6, 2020

alisman deleted the monorepo-docs-update branch January 6, 2020 22:06

inodb added the documentation label Jan 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improved monorepo docs #2969

improved monorepo docs #2969

onursumer commented Jan 3, 2020

inodb Jan 3, 2020

inodb Jan 3, 2020

inodb Jan 3, 2020

inodb Jan 3, 2020

inodb Jan 3, 2020

inodb left a comment

alisman commented Jan 6, 2020

Luke-Sikina left a comment

Luke-Sikina Jan 6, 2020

Luke-Sikina Jan 6, 2020

Luke-Sikina Jan 6, 2020

Luke-Sikina Jan 6, 2020


		Since now all the packages live under a single repository it might be confusing how to import different modules into the file you're working on.

		If you are working on `cbioportal-frontend` repository feel free to import any module from any package under `packages`. When importing from internal packages always use import statements like `import {something} from 'cbioportal-frontend-commons'`. Never do under any circumstances something like `import {something} from ../../packages/cbioportal-frontend-commons/src/lib/someLib` or `import {something} from ../../packages/react-mutation-mapper/src/component/someComponent`. Treat internal packages like any other npm package you would import.


		If you are working on `cbioportal-frontend` repository feel free to import any module from any package under `packages`. When importing from internal packages always use import statements like `import {something} from 'cbioportal-frontend-commons'`. Never do under any circumstances something like `import {something} from ../../packages/cbioportal-frontend-commons/src/lib/someLib` or `import {something} from ../../packages/react-mutation-mapper/src/component/someComponent`. Treat internal packages like any other npm package you would import.

		Avoid circular dependencies at all costs. For example, while it is okay to import a module from `cbioportal-frontend-commons` in `react-mutation-mapper`, there should not be any imports from `react-mutation-mapper` in `cbioportal-frontend-commons`. If you happen to need some component from from `react-mutation-mapper` in `cbioportal-frontend-commons`, consider moving that component into `cbioportal-frontend-commons` package.


		### Updating existing packages

		Whenever you need to update code under packages, you should also consider updating the version number in the corresponding `package.json` as well as the dependencies of other packages depending on the package you updated. For example if you update the `cbioportal-frontend-commons` version from`0.1.1` to `0.1.2`, corresponding `cbioportal-frontend-commons` dependency in the `package.json` for `react-mutation-mapper` and `cbioportal-frontend` should also be updated to the new version.

		@@ -271,6 +271,31 @@ In order to add a new workspace, one should create a new directory under `packag

		@@ -271,6 +271,31 @@ In order to add a new workspace, one should create a new directory under `packag

		Suggested way to add a new dependency to an existing workspace is to run `yarn workspace <workspace name> add <package name>` instead of just `yarn add <package name>`. For example, run `yarn workspace cbioportal-frontend add lodash` instead of `yarn add lodash`. Similarly to remove a package run `yarn workspace <workspace name> remove <package name>`.

		### Tips for dependency management


		Remember that the packages are used by other projects and compatibility needs to be carefully managed.

		Whenever you need to update code under packages, you should also consider updating the version number in the corresponding `package.json` as well as the dependencies of other packages depending on the package you updated. For example if you update the `cbioportal-frontend-commons` version from`0.1.1` to `0.1.2`, corresponding `cbioportal-frontend-commons` dependency in the `package.json` for `react-mutation-mapper` and `cbioportal-frontend` should also be updated to the new version.

improved monorepo docs #2969

improved monorepo docs #2969

Conversation

onursumer commented Jan 3, 2020

Checks

Any screenshots or GIFs?

Notify reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inodb left a comment

Choose a reason for hiding this comment

alisman commented Jan 6, 2020

Luke-Sikina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment