-
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 #2414
Comments
@gchoqueux First, thanks for your time writing a proposal. I have a few remarks that I would like to address. Geodesy packageI personally agree that we could first split the geodesy package (or geographic, let's have a vote folks) out of As I understand (from @mgermerie), the The I also agree that we should expose Widgets packageIs there any need to expose them in their own package if there are Utils packageI personally don't see any reason to expose them in their own package until we rewrote them from the ground up. Future roadmapThe more important point of this proposal is to discuss the future steps after splitting the |
I can easily make the
Future roadmap |
My 2 cents: It is a needed and valuable evolution (a monorepo architecture) but as we haven't plan (IGN / Ciril Group) to work on this subject in the short term, we need to sketch a plan for this migration before merging the creation of a first module so 1- we make sure we will be able to pick up the subject and finish it at some point 2- we don't create a first module that will change in the future 3- we don't start a migration that will stop after the first step and never finish it. Do you have any idea on how you would foresee the next steps of this separation? |
I add a road map with my next contributions |
hello everyone |
Thanks for adding a roadmap :) I have a few comments and questions:
That you will use the
Sorry I didn't really understand this part, can you give more details please? In my opinion we can start reviewing once we agree on the geodesy, widget and debug packages (since they are concerned by your PR). Regarding the rest, in general I would vote for a separation that is easy to understand (and use) for external users; roughly: a core - that can be separated in several sub-packages (geodesy being one of them) and it's that separation that we will mainly discuss I think -, and a separation by formats (@itowns/3dtiles, @itowns/potree, @itowns/wmts, etc.). I think that one way to go for the rest would be to:
I'll try to find time topropose something but don't hesitate to initiate it too. |
I could move this class, but I'm not sure where it will eventually end up. The Geodesic library could also include spatial elements (such as GNSS, satellite navigation).
ok
The idea is to create a class derived from This class could be used in
To simplify, this first PR will be limited to publishing two packages, but we can make it public without issue.
could give the entire path?
I expose in itowns to avoid breacking change, it's possible to add deprecated message
It's itowns Core package, almost all the iTowns libraries will depend on iTowns Core.
Maybe separate in thematic :
Yes, we could attach files when modules are valided
stateDiagram-v2
@itowns --> @itowns/core
@itowns --> @itowns/Loader
@itowns --> @itowns/geodesy
@itowns --> @itowns/widget
@itowns/Loader --> @itowns/3Dlayer
@itowns/geodesy --> @itowns/3Dlayer
@itowns/core --> @itowns/3Dlayer
@itowns/Loader --> @itowns/Rasterlayer
@itowns/geodesy --> @itowns/Rasterlayer
@itowns/core --> @itowns/Rasterlayer
@itowns/Rasterlayer --> @itowns/Terrain
@itowns/3Dlayer --> @itowns/Terrain
@itowns/3Dlayer --> @itowns/3DtilesLayer
@itowns/3Dlayer --> @itowns/PointCloudLayer
@itowns/core --> @itowns/viewer
|
Yes, or we could add all that in another module? No strong opinion on that, it could as well be in it. I'll let @Desplandis give his opinion too.
I agree that we should provide an easier API to position 3D objects in a geographic context as many users need to do so and it is the cause of many opened issues. Do you already have an example of how the API would look like in your opinion?
I'd directly make it public since it is mainly intended to external usage.
I don't understand your question? I mean moving them from
Are they already exposed in itowns package ? I thought they weren't. In that case, I would take them out and add a deprecation message yes.
Oh ok, I understand what you mean. We will need to discuss boundary cases but on a general view I agree on what you listed to be in it.
Yes, 3D tiles and potree are easy cases because the source and the layer need to be used together 😁 Some sources can indeed be used with different layers - e.g. WMTS with
That being said, I think separating in thematics won't resolve these cases either I think: where would you put
I like the third option better. What do you think ? Do you see other alternatives?
Thanks! Can you use an open tool and maybe something that can be edited online so we can collaborate on it? I know draw.io but you can choose another one that suits you. |
Ok, I Move
THREE.Group with the I remove this part from the proposal because it's more a refactoring/feature and because it's too precise
I move
I had proposed to put it in the @itowns/loaders, which amounts to separating the sources as in your third proposal but with a different granularity (Both are even possible). I will continue working on the segmentation in a shared diagram
To start the code review and avoid continuously rebasing the branch, we need to decide on the PR structure Geodesy Which modules do you approve ? |
This comment was marked as duplicate.
This comment was marked as duplicate.
Hi @gchoqueux , thanks for making a diagram and re-working the proposal! 👍 Sorry I add a busy week, I will take a look at it at the beginning of next week. |
ok |
@iTowns/core-devs I've rewritten the initial proposal, removing the PR-related specifics (#2300) and focusing on the architecture to be validated. |
@gchoqueux thanks for the work, it's way clearer now! I'm waiting for incoming @Desplandis review before reviewing the PR. Regarding the proposal, I agree with most of what is described in it. The only remarks that I have are:
All the rest looks great 👍 |
Adopt a monorepo structure for itowns. The goal is to split itowns down into reusable and standalone feature packages.
As an example, I've started with the geodesy features. (@itowns/geodesy)
Context
For the moment, itowns provides an api in a single package.
this architecture poses several problems :
Viewer
instance.Description of the proposal
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 features.
This structure makes it necessary to make features independent and improves the architecture
For the moment only one new package is proposed: Geodesy
Identified use-cases
Users can use only the packages they are interested in.
Three.js developers can use itowns features more easily.
Developers more easily merge their codes (I received feedsbacks for this architecture)
Implementation
see #2300
Comments
This monorepo structure supports Yarn
There is not breaking change for itowns users
Potential Problems
Rebasing but generally Git does the job, Except when there are bumps and new files or move files
The new publishing, only tested on my npm account
iTowns Monorepo Structure Proposal
1.⚠️
@itowns/core
:2.⚠️
@itowns/geodesy
4.
@itowns/sources
@itowns/sources/wms
@itowns/sources/wmts
@itowns/sources/3dtiles
@itowns/sources/gpx
@itowns/sources/kml
4.
@itowns/layers
@itowns/layers/raster
@itowns/layers/colorLayer
@itowns/layers/colorElevation
@itowns/layers/vector
@itowns/sources/3dtiles
@itowns/sources/featureGeometryLayer
@itowns/layers/terrain
5.
@itowns/mouseControls
@itowns/controls/Globe
@itowns/controls/Planar
@itowns/controls/Immersion
@itowns/controls/firstperson
6.⚠️
@itowns/widget
@itowns/widget/navigation
@itowns/widget/minmap
@itowns/widget/scale
6.
@itowns/plugins
@itowns/plugins/debug
: dev debug tools@itowns/plugins/measure
: Measurement tools (distance, area)@itowns/plugins/analysis
: Analysis tools (elevation profiles, cut sections)To validate
The first validation step is the specified modules with the logo⚠️ that are included in the PR #2300.
The Following modules must be validated to begin the PR review :
@itowns/core
@itowns/geodesy
@itowns/widget
@itowns/plugins/debug
The text was updated successfully, but these errors were encountered: