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

Monorepo structure #3656

Closed
jfirebaugh opened this issue Nov 18, 2016 · 11 comments
Closed

Monorepo structure #3656

jfirebaugh opened this issue Nov 18, 2016 · 11 comments

Comments

@jfirebaugh
Copy link
Contributor

How should we organize the monorepo? Let's start with mapbox-gl-js, mapbox-gl-test-suite, mapbox-gl-style-spec, and mapbox-gl-shaders.

Of these, the only two we currently publish as independent packages are mapbox-gl-js and mapbox-gl-style-spec. mapbox-gl-native will depend on a mapbox-gl-js commit hash and pull style-spec, shaders, and test-suite from there.

Possible structures include the following

The "checked in node_modules" approach

  • mapbox-gl-js
    • bench, debug, js, plugins, etc.
    • node_modules
      • mapbox-gl-test-suite
      • mapbox-gl-style-spec
      • mapbox-gl-shaders

I haven't had good experiences with checking in portions of node_modules, so I'm not inclined to try this approach.

The "semi-lerna" approach

  • mapbox-gl-js
    • bench, debug, js, plugins, etc.
    • packages
      • mapbox-gl-test-suite
      • mapbox-gl-style-spec
      • mapbox-gl-shaders

Here we'd keep the existing mapbox-gl-js structure, but use lerna to manage the integrated dependencies. I don't know if lerna actually supports this.

The "full-lerna" approach

  • mapbox-gl
    • packages
      • mapbox-gl-js
        • bench, debug, js, plugins, etc.
      • mapbox-gl-test-suite
      • mapbox-gl-style-spec
      • mapbox-gl-shaders

I believe this is how lerna is intended to be used.

The "full integration" approach

  • mapbox-gl
    • bin (merged from mapbox-gl-js and mapbox-gl-style-spec)
    • bench, debug, js, plugins, etc. (from mapbox-gl-js)
    • lib, reference, migrations, etc. (from mapbox-gl-style-spec)
    • shaders
    • test (merged from mapbox-gl-js and mapbox-gl-style-spec)
    • test-suite
      • data, query-tests, render-tests, etc.
    • package.json (merged from all repos)

The idea here is that we'd fully integrate all four repos, with extensive merging and massaging of their structure. We'd stop publishing mapbox-gl-style-spec as an independent package.

This feels like the right approach to me. It's also most likely the most amount of work.

Whatever structure we decide on, I think we should fully script the restructuring, so that we can cleanly cut over from the current state of all the repos without a period where we have to maintain two structures.

Thoughts?

@jfirebaugh
Copy link
Contributor Author

Drawback of the "full integration" approach: the package.json would get bloated with non-dev dependencies that are only needed for the test-suite, shaders, or style-spec.

@lucaswoj
Copy link
Contributor

My first impression is that it'd be easiest to start with the "checked in node_modules" approach and transition to the "full integration" approach iteratively.

@tmcw
Copy link
Contributor

tmcw commented Nov 18, 2016

Lerna has been perfectly nice for Turf, but I don't think it would really pay off in this situation given the relatively few consumers of style-spec, shaders, and function. It definitely does introduce some uncertainty and complexity into the situation, since it's designed for dependencies to be versioned semi-independently too. The full integration approach feels right to me, but that may just be because I've never worked on or encountered it in production.

@mourner
Copy link
Member

mourner commented Nov 18, 2016

Very much in favor of doing this — we could get a huge productivity boost!

Not a fan of the node_modules approach. I think the "full integration" path is the best one, although we should see if it's worth merging the git history as well, or whether we'll just start mapbox-gl-test-suite, mapbox-gl-style-spec and mapbox-gl-shaders histories from scratch while archiving the old repos for history reference.

Before we start merging the repos, we need to do some clean up first — going through all dependencies, making versions consistent with those in gl-js: e.g. style-spec uses ancient eslint 0.16; test-suite still uses handlebars although we switched to a syntax-compatible lodash-template in gl-js; style-spec uses tape and istanbul although we use tap and nyc in gl-js; we could also possibly eliminate some non-essential dependencies. After we clean up, the bloat shouldn't be that bad.

@mikemorris
Copy link
Contributor

mikemorris commented Nov 18, 2016

Drawback of the "full integration" approach: the package.json would get bloated with non-dev dependencies that are only needed for the test-suite, shaders, or style-spec.

My concern here would be conflating the package.json from the Node.js bindings (published on npm as mapbox-gl-native) with that of gl-js. Is there any sort of directory structure that would allow separate package.json files somewhere other than the root level?

EDIT: Rereading, sounds like we wouldn't be including mapbox-gl-native in the scope of this monorepo?

mapbox-gl-native will depend on a mapbox-gl-js commit hash and pull style-spec, shaders, and test-suite from there

^ seems a bit awkward

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 18, 2016

Drawback of the "full integration" approach: the package.json would get bloated with non-dev dependencies that are only needed for the test-suite, shaders, or style-spec.

Would this be so bad?

Here's a full list of non-dev dependencies that would be added from a naive merge of mapbox-gl-style-spec and mapbox-gl-shaders. Several of the below dependencies and all the dependencies of mapbox-gl-test-suite could be made devDependencies.

{
  "csscolorparser": "~1.0.2", // already in GL JS
  "fast-stable-stringify": "^0.1.1",
  "jsonlint-lines-primitives": "~1.6.0", // could be a dev dependency
  "lodash.isequal": "^3.0.4", // could be replaced by GL JS's util library
  "minimist": "0.0.8", // could be a dev dependency
  "rw": "^0.1.4",
  "sort-object": "^0.3.2",
  "brfs": "^1.4.0"
}

@jfirebaugh
Copy link
Contributor Author

Several of the below dependencies and all the dependencies of mapbox-gl-test-suite could be made devDependencies.

The problem is they can't, really; not if mapbox-gl-native is going to consume the monorepo as a dependency and use it to run the test-suite.

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 21, 2016

Ah. That is tricky. Because this is an internal edge case, we could consider a workaround like

(cd node_modules/mapbox-gl-js && npm install)

@lucaswoj
Copy link
Contributor

lucaswoj commented Dec 7, 2016

It feels like an actionable path forward is merging here. Remaining questions for all:

  • How do we feel about adopting the devDependencies workaround proposed above ☝️?
  • How do we feel about progressively implementing the "full integration" approach proposed above, merging one external project at a time? (one PR which merges mapbox-gl-test-suite, one PR which merges mapbox-gl-style-spec, ...)
  • Is there a sensible way to preserve git history with a "full integration" approach? cc @jfirebaugh

@lucaswoj
Copy link
Contributor

lucaswoj commented Feb 6, 2017

This is mostly done. Specific follow-ups are ticked separately.

@lucaswoj lucaswoj closed this as completed Feb 6, 2017
@mollymerp
Copy link
Contributor

amazing 💪 @lucaswoj !! 🙇‍♀️

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

6 participants