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

De-duplicate devDependencies #4362

Closed
pshrmn opened this issue Jan 22, 2017 · 26 comments
Closed

De-duplicate devDependencies #4362

pshrmn opened this issue Jan 22, 2017 · 26 comments
Labels

Comments

@pshrmn
Copy link
Contributor

pshrmn commented Jan 22, 2017

With the monorepo, each package is installing its devDependencies in its own node_modules. For me, this led to the react-router folder weighing in at ~234MB. A lot of these are duplicated across packages and should be able to be moved to the root package.json. The only ones that can't, as noted by the lerna documentation are the ones that need binaries to run (i.e., karma).

For reference, across each package.json each dependency is included the following number of times:

Package Count
babel-cli 2
babel-core 1
babel-eslint 2
babel-loader 3
babel-plugin-dev-expression 2
babel-plugin-transform-react-remove-prop-types 2
babel-preset-es2015 3
babel-preset-react 3
babel-preset-stage-1 3
bundle-loader 1
copy-webpack-plugin 1
css-loader 1
eslint 2
eslint-plugin-import 2
eslint-plugin-react 2
expect 2
file-loader 1
gzip-size 2
html-loader 1
html-webpack-plugin 1
in-publish 2
jsxstyle 1
karma 2
karma-browserstack-launcher 2
karma-chrome-launcher 2
karma-mocha 2
karma-mocha-reporter 2
karma-sourcemap-loader 2
karma-webpack 2
markdown-it 1
markdown-it-anchor 1
mocha 2
postcss-loader 1
pretty-bytes 2
prismjs-loader 1
react 4
react-addons-test-utils 2
react-dom 3
react-motion 1
react-native 1
readline-sync 2
style-loader 1
sw-precache 1
sw-precache-webpack-plugin 1
url-loader 1
webpack 3
webpack-dev-server 3
@mjackson
Copy link
Member

Ya, there's a bunch of build stuff that can be de-duplicated, though I'm not terribly concerned about it right now. Just trying to get modules (and tests for them!) into the right packages right now. We can optimize the repo structure later.

@mjackson
Copy link
Member

Blargh, 7e2ba75 caused some unforeseen issues. karma, in particular, has trouble finding its plugins unless they're all installed together. Gonna revert and maybe take another look at some point in the future.

@mjackson mjackson reopened this Jan 23, 2017
@mjackson
Copy link
Member

Actually, lerna/lerna#507 should take care of this. When that lands, let's revisit.

@timdorr timdorr added this to the v4.0.0 milestone Feb 1, 2017
@timdorr
Copy link
Member

timdorr commented Feb 1, 2017

Beta 35 is out, so this is ready to go: https://twitter.com/left_pad/status/826857968930992128

This was referenced Feb 1, 2017
@cpeddecord
Copy link

Can work on this, have a monorepo at work I maintain that needed a similar solution.

@mjackson
Copy link
Member

mjackson commented Mar 7, 2017

Thanks, @cpeddecord, that would be really helpful. The work needs to happen in the lerna repo, not here. They have a fairly active Slack channel if you're interested. Please ping us if anything changes.

For now, I'm going to remove this from the 4.0.0 milestone. It's inconvenient, but not a blocker for shipping v4.

@mjackson mjackson removed this from the v4.0.0 milestone Mar 7, 2017
@cpeddecord
Copy link

@mjackson oh interesting, I was going to move all devDeps into the root repo and out of each package. All testing/linting/building/etc then occurs in the root repo and is no longer the responsibility of the package save for outlier tasks, such as the docs site.

@mjackson
Copy link
Member

mjackson commented Mar 8, 2017

@cpeddecord That works for most things except for karma, AFAIK, which doesn't work for some reason when it's like that. See #4362 (comment)

@cpeddecord
Copy link

cpeddecord commented Mar 8, 2017

@mjackson I've worked around it by something like this, works fine if a bit ugly:

"scripts": {
  "test":"node ../../node_modules/karma/bin/karma.js start --single-run"
}

@cpeddecord
Copy link

@mjackson Cool, if it's re-opened I'll spend some time on this, sharing tooling etc., :)

@mjackson
Copy link
Member

mjackson commented May 1, 2017

Thanks, @cpeddecord. That would be great :)

@timdorr
Copy link
Member

timdorr commented Jul 15, 2017

Now that we're up to lerna@2.0.0, they supposedly have this fixed. Now's as good a time as any to see if it works! 👷‍♀️

@mjackson
Copy link
Member

If it does, I think we'll be able to drastically reduce our build times.

@cpeddecord
Copy link

started work on this over the weekend, wondering how you feel about a unified build task versus distributed.

instead of lerna run build it'd be node ./scripts/build to kick off a build process that was duplicated between the various packages.

@timdorr
Copy link
Member

timdorr commented Jul 17, 2017

That's kind of one of the big reasons for using lerna. They also manage concurrency for us. I'd rather not own this ourselves.

@mjackson
Copy link
Member

@cpeddecord I agree with @timdorr. I'd prefer each package to have its own scripts as much as possible. That way if they differ we won't have to make exceptions for specific packages.

@mjackson
Copy link
Member

mjackson commented Sep 8, 2017

I'd like to try Yarn workspaces in this repo and move away from lerna. We're not using any of lerna's more esoteric features, so it seems like workspaces would do the trick.

@timdorr
Copy link
Member

timdorr commented Sep 8, 2017

Lerna should work with workspaces. It was mentioned in the 1.0 announcement. They also said to take a look at babel's transition to workspaces. There's a lot of noise in there, but it seems to be simple enough.

@mjackson
Copy link
Member

mjackson commented Sep 8, 2017

If we're using workspaces, the only thing we'll need lerna for is the publish step.

@pshrmn
Copy link
Contributor Author

pshrmn commented Sep 8, 2017

Perhaps I'm just missing seeing it in the announcement, but with workspaces can you yarn run <script name> in the root directory to run all of the tests, builds, and clean?

@pshrmn
Copy link
Contributor Author

pshrmn commented Sep 8, 2017

Five minutes after I posted that, #5502 was opened, which uses jest --projects (Jest 20 feature) to get rid of the concern with testing all of the packages.

I'm still not sure about the build step. A custom build script could work, but it might be easiest to get workspaces working first, then figure out how to drop lerna.

As an aside, would it make sense to move react-router-website out of the packages? It isn't really a package, so maybe it would be more appropriate to keep it in a website directory?

@timdorr
Copy link
Member

timdorr commented Sep 8, 2017

As an aside, would it make sense to move react-router-website out of the packages? It isn't really a package, so maybe it would be more appropriate to keep it in a website directory?

Yes plz.

@mjackson
Copy link
Member

mjackson commented Sep 8, 2017

Yes, I'd agree we get workspaces working first with lerna, then figure out what to do from there.

Is react-router-website causing problems? It has a { "private": true } flag in its package.json, so I didn't think it'd cause any problems with publishing. As for installing deps and running tests, it works like any other package. That's why I put it there.

@pshrmn
Copy link
Contributor Author

pshrmn commented Sep 8, 2017

For me, moving react-router-website out of packages is more of a perception issue. When I think about a monorepo, I think of the packages directory being the code that could be on npm. It might not be published there (private: true), but it could be. With the documentation website, that is something that would never be published, so for me it doesn't feel right being in the packages directory.

While I don't imagine most people ever need to go looking at the doc site's source, there have been issues opened when people have wanted to, but didn't know where the source was (#5440). I think that having a website folder in the root would make this quite obvious.

One other annoyance with it (and this could be removed if we dropped lerna and added a custom build) is that we are building the documentation website whenever we bootstrap the repo. That means that whenever the Travis tests are run, we are wasting time building the website. I think that it makes sense to only install the deps needed for the website when someone actually wants to run the website locally.

Finally, I just took a look at the monorepos listed in that announcement and how they store their documentation website.

  1. Babel uses a different repo
  2. React uses a root docs directory (which gets built and stored in the gh-pages branch).
  3. Jest uses a root website directory
  4. Vue uses a different repo
  5. Angular uses a root aio directory (that took me a minute to find, but I guess aio stands for angular.io)

This topic is only partially related to this actual issue, so I'll try not to derail this any more, but I just wanted to post this food for thought.

@mjackson
Copy link
Member

@pshrmn You bring up some good points. Of all those options, I like the idea of a docs/website dir the most. Separate repos are a pain to setup and maintain. (Side note: what if every line of code I ever wrote was all in the same repo??? but I digress...)

Since having the website in a separate dir is probably going to change some of the top-level scripts and the way we install things, I'll do that work in the same commit. I tested out yarn workspaces the other day on another project and it worked really well.

mjackson added a commit that referenced this issue Sep 15, 2017
The react-router-native tests don't pass anymore on master, so I'm
reverting this until we can figure out why.

Re-opens #4362
@mjackson mjackson reopened this Sep 15, 2017
mjackson added a commit that referenced this issue Sep 15, 2017
pshrmn pushed a commit that referenced this issue Sep 16, 2017
@timdorr
Copy link
Member

timdorr commented Nov 13, 2017

I guess this is done now with #5720 👍

@timdorr timdorr closed this as completed Nov 13, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants