-
Notifications
You must be signed in to change notification settings - Fork 303
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
Monorepo structure #2300
base: master
Are you sure you want to change the base?
Monorepo structure #2300
Conversation
9431214
to
e23cfef
Compare
@Desplandis I resolve some issues
|
@gchoqueux Nice! I will take a quick look of your changes and write a todo list of things that should absolutely be tested before reviewing more thoroughly this PR. Expect it for tomorrow or next tuesday! @jailln @mgermerie @ftoromanoff @AnthonyGlt I think we'll need your inputs on this change! ;) This should be a good start to fix #2197, #1930, #2201 (list not exhaustive, feel free to complete). Shall we create a meta-issue to aggregate all those issues? |
e5ec2ea
to
b2308ff
Compare
ea0116d
to
962c99d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much to add to @Desplandis review.
I just think we should explain this new structure in the README and add sub package installation possibilities.
Also, how does it work with releasing and versioning? does subpackage all have the same version or can we (should we?) release them independently ?
ac0a1ad
to
451da03
Compare
Yes, I'll document
In a monorepo, it's more complicated to manage a different version for each module, but if we can find a simple and robust solution we can implement it later. |
d4616ba
to
b1d0ba1
Compare
@gchoqueux Did you try to publish it in your own This is the a log of my last attempt: The CI works correctly but I would like to ensure that both npm and website publishing are working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last batch of comments for me, only documentation remarks, I let you see with @Desplandis for package publishing. Thanks for the changes you already made
```bash | ||
npm install --save itowns | ||
``` | ||
|
||
```js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to replace by something more generic and that will be easier to update when new packages are added and to remove examples since we already have examples in each sub-package documentation:
iTowns is currently moving to a monorepo organization and to a segmentation in sub-modules, allowing to import only some of itowns functionalities. Current itowns sub-modules are:
- [@itowns/geographic](packages/Geographic/README.md): `npm install --save @itowns/geographic`
- [@itowns/widgets](packages/Widgets/README.md): `npm install --save @itowns/widgets`
- [@itowns/core](packages/Main/README.md); `npm install --save @itowns/core`
Note the s at the end of @itowns/widgets
since there is more: can you update this package name and also the folder name: packages/Widget -> packages/Widgets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rename packages/Widget
to packages/Widgets
@itowns/widgets is private and @itowns/core doesn't exist for the moment. I could remove the sub-modules examples ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! Ah yes ok sorry I thought we chose to make it public in this PR and didn't look if it was the case. Making it public now means also improving the doc, so I guess we can do that in a subsequent PR so we can merge this PR quicker
Ok for @itowns/core
, my bad. So yes, only that is sufficient in my opinion:
iTowns is currently moving to a monorepo organization and to a segmentation in sub-modules, allowing to import only some of itowns functionalities. Current itowns sub-modules are:
- [@itowns/geographic](packages/Geographic/README.md): `npm install --save @itowns/geographic`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
a6179f1
to
3569118
Compare
You need to register the packages, it seems to manage dependencies as a single package. I think I used a publish-latest in local to register all subpackages. The deploying an releasing work see jobs I'm trying to solve this problem, and I think I've got a lead. |
f902474
to
582a15a
Compare
582a15a
to
7f00fd3
Compare
Description
Refactoring iTowns to monorepo structure.
The goal is to split itowns down into feature packages.
As an example, I've started with the geographic functionalities. (@itowns/geographic)
create organization NPM to support @iTownsexportsFields
For the moment
Debug
andWidget
modules are private.Motivation and Context
Split code in packages for clearly structured code.
Simplifies development and facilitates contributions.
Increases the scope of users who only want to use a few functions.
This structure makes it necessary to make functions independent
Code movement
The classes
Coordinates, Crs, Ellipsoid, Extent, OrientationUtils
files are moved to./packages/geographic/src
.The
proj4
dependency moves to geographic package.The unit tests moves to
packages/xxx/test
.the code
utils/debug
movespackages/Debug
the code
src/Utils/Gui
movespackages/Widget
dependencies
This proposal from monorepo doesn't use monorepo package (by example Lerna).
New dev dependency is
Concurrently
, it's used to watch script in parallelItowns dependencies
(three, proj4...) are moved to the packages that use themnode scripts
The scripts uses the options
--workspaces
or-ws
, to call the corresponding script in each sub packages.