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

Initial monorepo configuration #419

Closed
wants to merge 10 commits into from
Closed

Conversation

ryanyogan
Copy link

@ryanyogan ryanyogan commented Aug 11, 2016

This PR augments the directory structure to conform to a multi-package "monorepo"

Tree structure changes

create-react-app/
+-- .travis.yml
+-- CHANGELOG.md
+-- CONTRIBUTING.md
+-- LICENSE
+-- PATENTS
+-- README.md
+-- packages/
    +-- create-react-app/  [originally global-cli]
    |   +-- index.js
    |   +-- package.json  [maintaining independent packages]
    +-- react-scripts/  [originally root directory]
    |   +-- bin/
    |   +-- config/
    |   +-- scripts/
    |   +-- template/  [originally in root directory]
    |   +-- .eslintrc.js
    |   +-- .gitignore
    |   +-- package.json [maintaining independent packages]
+-- tasks/
|   +-- e2e.sh
|   +-- release.sh

Status

Given this PR modifies a good portion of the application structure, it may go out of sync quickly.

This PR has been updated to reflect:
master: 7c912b5ffe48eda1c8b0b6f005a117d5764eb0a6
on August 15th, 2016 at 12:15PM PDT

Please ping @ryanyogan if you are reviewing and it is no longer in sync.

How to test

  • From the root directory run sh tasks/e2e.sh or cd packages/react-scripts && npm run e2e
  • Manual install of CLI
    • cd packages/create-react-app
    • npm uninstall create-react-app -g we need to remove your official package temporarily
    • npm i . -g <-- this command will install from the local directory
    • Note: npm uninstall create-react-app -g when complete, you don't want this test to clash with the official package. Now you can go back to the official npm i create-react-app -g
  • Further testing
    • cd packages/create-react-app && npm i cd packages/react-scripts && npm i && npm run create-react-app test-app
    • Test the build process: npm run build
      • You will now have a build folder in the root directory, don't forget to trash it.
    • Remember this will create an application named test-app in the packages/react-scripts directory, you will want to trash that once complete. You will also have a node_modules folder in both the packages/create-react-app folder and the packages/react-scripts folder, you will want to delete them with rm -rf node_modules in the respective directory.

Noteable changes / todo's

  • I made changes that hop around directories in the tasks/e2e.sh file, I am not happy with them I think this file can be improved upon.
  • Splitting up the paths file into separate modules in addition to making it a tad smarter would help in the future.
  • I started toying around with Lerna.js in "independant" mode per @hzoo's advice and I found it to be quite nice. Perhaps this would be a nice next step, however I see other issues that should be cleaned up first.

@ghost
Copy link

ghost commented Aug 11, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost
Copy link

ghost commented Aug 11, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added CLA Signed labels Aug 11, 2016
@@ -20,6 +20,7 @@
"files": [
"bin",
"config",
"PATENTS",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not move the PATENTS file, it must stay at the top of the repo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, sorry about that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! No worries, some of the legal related things unfortunately need some special treatment :)

@gaearon
Copy link
Contributor

gaearon commented Aug 13, 2016

Sorry for the trouble, mind rebasing? Thanks!

@ryanyogan
Copy link
Author

@gaearon No problem at all

@ghost ghost added the CLA Signed label Aug 13, 2016
@ryanyogan
Copy link
Author

@gaearon I'm trying to be a good citizen keeping this in sync, given this PR touches so much surface area it is going to get out of sync pretty easily. I rebased again, just ping me if this is going to sit for a while and I will rebase once / if it goes through review. Thanks!

@ghost ghost added the CLA Signed label Aug 15, 2016
@ryanyogan
Copy link
Author

Issue #450 will cause an issue with this as well, do you prefer I dump the README.md in the /packages/create-react-app directory and put a more generic one in the root directory?

@gaearon
Copy link
Contributor

gaearon commented Aug 22, 2016

Sorry, I’ve been jumping between projects lately, and CRA is currently on hold until enough fixes accumulate. I’ll get back to this when I’m ready to cut 0.3.0 (or maybe even get right to 1.0.0).

@gaearon gaearon added this to the 0.3.0 milestone Aug 22, 2016
@ghost ghost added the CLA Signed label Aug 22, 2016
@gaearon gaearon modified the milestones: 0.4.0, 0.3.0 Sep 1, 2016
@ghost ghost added the CLA Signed label Sep 1, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

I would like root README.md to end up in the create-react-app npm on its publish, but keep it in the repo root. Can you figure out how to do this?

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

I also just merged #257 which changes some things so you’ll need to rebase again, but I’m ready to get this in right after that. Sorry about the back and forth. It’s a massive change and I appreciate the effort.

@ryanyogan
Copy link
Author

No problem, I'll look into that today and rebase up as well.

Cheers

On Fri, Sep 2, 2016 at 8:50 AM, Dan Abramov notifications@github.com
wrote:

I also just merged #257
#257 which
changes some things so you’ll need to rebase again, but I’m ready to get
this in right after that. Sorry about the back and forth. It’s a massive
change and I appreciate the effort.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#419 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACifA2hBe0AOMkz_9qjgAEjVQRqHlvLks5qmEWqgaJpZM4JhyYv
.

@gaearon gaearon modified the milestones: 1.0.0, 0.4.0 Sep 2, 2016
@ghost ghost added the CLA Signed label Sep 2, 2016
@ryanyogan
Copy link
Author

wow! Lot's of awesome changes! I need to parse through some of this, I may need to take this on tomorrow. I'll try and get it out ASAP.

@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

@ryanyogan This is blocking some future improvements I want to make (since they’ll need new packages). Can you please let me know if you can revisit this? Let us know if you’re too busy and we could ask somebody else to try to get it to a mergeable state. Your help is very much appreciated either way. Thanks!

@gaearon gaearon modified the milestones: 0.5.0, 1.0.0 Sep 16, 2016
@ghost ghost added the CLA Signed label Sep 16, 2016
@ryanyogan
Copy link
Author

Hi Dan, sorry I have been flying around for some interviews, I can give
this my full attention tomorrow to finish if that is too late by all means
I can push up my broken branch and someone else can take it.

On Fri, Sep 16, 2016 at 5:04 PM, Dan Abramov notifications@github.com
wrote:

@ryanyogan https://github.com/ryanyogan This is blocking some future
improvements I want to make (since they’ll need new packages). Can you
please let me know if you can revisit this? Let us know if you’re too busy
and we could ask somebody else to try to get it to a mergeable state. Your
help is very much appreciated either way. Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#419 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACifESY-PxhKfb95v4Nu9q2FngzKw0Rks5qqxJSgaJpZM4JhyYv
.

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

@ryanyogan If you haven’t had time yet, feel free to push work in progress and I’m sure somebody can pick it up!

@ghost ghost added the CLA Signed label Sep 18, 2016
@ryanyogan
Copy link
Author

Will do, sorry, it's been a crazy week.

On Sep 18, 2016 12:11 PM, "Dan Abramov" notifications@github.com wrote:

@ryanyogan https://github.com/ryanyogan If you haven’t had time yet,
feel free to push work in progress and I’m sure somebody can pick it up!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#419 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACifG87Ua7vLp-YyHkbH9mEUcRkzqUQks5qrYzxgaJpZM4JhyYv
.

@ghost ghost added the CLA Signed label Sep 18, 2016
@fson fson mentioned this pull request Sep 19, 2016
6 tasks
@gaearon
Copy link
Contributor

gaearon commented Sep 19, 2016

This is now in. Thanks!

@gaearon gaearon closed this Sep 19, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants