itowns icon indicating copy to clipboard operation
itowns copied to clipboard

Monorepo structure

Open gchoqueux opened this issue 2 years ago • 16 comments

Description

Refactoring iTowns to monorepo structure.

The goal is to split itowns down into feature packages. As an example, I've started with the geodesy functionalities. (@itowns/geodesy) ~I use lerna to watch and build packages while the dev server is in use, but I'm not completely satisfied because the files aren't supplied via memory and files are also transpiled into the browser's debugger. Lerna is also used to test packages.~

  • [x] move unit test
  • [x] improve dev server
  • [x] babel resolver plugin is broken
  • [x] documentation
  • [x] coverage
  • [x] bump version
  • [x] move all src in packages
  • [x] watch src packages
  • [x] publishing, auto publish with github actions
  • [ ] create organization NPM to support @itowns
  • [x] move dependencies to packages
  • [x] webpack : use alias to resolve dependencies instead of exportsFields
  • [x] use babel monorepo configuration
  • [x] move export from private root package.json
  • [x] write all readme.md files

For the moment Debug and Widget 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

gchoqueux avatar Mar 21 '24 14:03 gchoqueux

@Desplandis I resolve some issues

  • resolve module with exportsFields option
  • remove lerna with --workspace cli option

gchoqueux avatar Mar 27 '24 17:03 gchoqueux

@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?

Desplandis avatar Mar 27 '24 20:03 Desplandis

I'll we check it out once the PR is reopen, thanks for the work

AnthonyGlt avatar Apr 18 '24 13:04 AnthonyGlt

@gchoqueux I managed to correctly configure babel without having to resort to non-standard configurations in webpack. With my fixes, we just need to add an alias in the webpack configuration.

Shall I wait for further changes to push those commits?

P.S.: I even managed to convert one of the subpackages to typescript by changing its local babel configuration.

Desplandis avatar Apr 30 '24 11:04 Desplandis

note that the Debug package is in private

gchoqueux avatar Jun 21 '24 13:06 gchoqueux

I propose a live presentation to explain this PR

gchoqueux avatar Jun 21 '24 13:06 gchoqueux

Branch rebase on master Webpack serve was fixed

gchoqueux avatar Jun 27 '24 13:06 gchoqueux

I propose a live presentation to explain this PR

In my opinion the first thing to do is to agree together on the modules separation (i.e. on a final expected list of sub modules) and on the way / order in which it should be done to avoid going back and forth and so that anyone can contribute to this subject. We can discuss it all together in a call first if you want but I think it will need to end up in an issue eventually. WDYT?

jailln avatar Jul 04 '24 10:07 jailln

Yeah, I agree with @jailln and think we should start an issue/proposal where we could centralize all discussions on package splitting (use-cases, explanation of architectural/tooling changes, a sort of informal roadmap, ...). As long as we discuss those topics, I'm all in for a presentation.

@ftoromanoff, @mgermerie and @AnthonyGlt, I think you should participate too. =)

Desplandis avatar Jul 04 '24 10:07 Desplandis

in this PR the modules follow the itowns structure

  • itowns : already existing module
  • widget : already in separate structure in src/Utils/gui and a specific bundle itowns_widgets, in this PR this module is private and it isn't published
  • debug : already in separate structure in utils/debug and a specific bundle debug, in this PR this module is private and it isn't published
  • geodesy : new module to measure and represente the geometry, gravity, and spatial orientation of the Earth in temporally varying 3D.

gchoqueux avatar Jul 04 '24 12:07 gchoqueux

in this PR the modules follow the itowns structure

  • itowns : already existing module
  • widget : already in separate structure in src/Utils/gui and a specific bundle itowns_widgets, in this PR this module is private and it isn't published
  • debug : already in separate structure in utils/debug and a specific bundle debug, in this PR this module is private and it isn't published
  • geodesy : new module to measure and represente the geometry, gravity, and spatial orientation of the Earth in temporally varying 3D.

Ok thanks, this is fine by me. I would just rename geodesy to geographic, WDYT? What we should discuss/list in an issue is what are the next steps (next modules) and in which order we should split them

jailln avatar Jul 04 '24 12:07 jailln

geodesy includes gravity, orientation, coordinates. Even if the name is less well-known, it's the right term used by professionals. I think it's worth communicating about these rare features.

gchoqueux avatar Jul 04 '24 13:07 gchoqueux

I rebased this PR who could review it ?

gchoqueux avatar Sep 25 '24 11:09 gchoqueux

Yeah, I agree with @jailln and think we should start an issue/proposal where we could centralize all discussions on package splitting (use-cases, explanation of architectural/tooling changes, a sort of informal roadmap, ...). As long as we discuss those topics, I'm all in for a presentation.

Once again, I reiterate the needs for a proper roadmap for package splitting before doing any kind of review (note that I can't do it nonetheless since I participated initially in the code of this PR). This is in our actual governance document to first open a proposal before validating any new functionality.

I know personally that there is some needs from IGN research team to split geographic package from the rest of itowns but we can't have an half-backed package splitting.

Finally, we have some recent PRs (refactoring and migration from js to ts) that are conflicting with the 300+ files changed (quite a huge PR in my opinion), so if you can't commit to open a discussion soon enough we'll surely merge them before this PR.

Pinging once again @jailln @AnthonyGlt @mgermerie @ftoromanoff.

Desplandis avatar Sep 25 '24 12:09 Desplandis

o if you can't commit to open a discussion soon enough we'll surely merge them before this

This is a valuable PR that we would like to integrate as soon as possible but I agree with @Desplandis that, as said before, we should follow our new contribution workflow because it is an impacting change. It boils down to opening a proposal issue describing the changes and proposing a plan (or a draft) to complete this migration (this PR being one part of it) and discussing it together so we all agree beforehand. This won't necessary take long and we can be reactive on this discussion so we can finish this first step quickly.

jailln avatar Sep 25 '24 12:09 jailln

I open #2414

gchoqueux avatar Sep 25 '24 13:09 gchoqueux