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 (WIP) #164

Merged
merged 11 commits into from
Oct 23, 2016
Merged

Monorepo (WIP) #164

merged 11 commits into from
Oct 23, 2016

Conversation

DxCx
Copy link
Contributor

@DxCx DxCx commented Oct 4, 2016

TODO:

Closes #138
Closed #163

@DxCx
Copy link
Contributor Author

DxCx commented Oct 4, 2016

Hi @nnance (@helfer pointed at you), @stubailo
I've done intial work for monorepo, still have small stuff to close. (TODO on PR)
but would like to get your first feedback

@nnance
Copy link
Contributor

nnance commented Oct 6, 2016

@DxCx sorry I just now got the time to look this over. I will give feedback as soon as I finish an initial review. With that being said I can't seem to get the project to install. What's the process for getting the project initialized?

@DxCx
Copy link
Contributor Author

DxCx commented Oct 6, 2016

Just 'npm install' ;)
The mono repo init is a postinstall script

@DxCx DxCx force-pushed the monorepo#138 branch 2 times, most recently from 714be45 to fcee833 Compare October 6, 2016 12:31
@DxCx
Copy link
Contributor Author

DxCx commented Oct 6, 2016

ok guys, i'm done with my items. everything seems to work against private npm repo.
waiting for more feedback now and the reset of todo items

Copy link
Contributor

@nnance nnance left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. My original problem with npm install failing was because I had nvm set to Node v4.

I have one more issue I am looking into in my environment. I am running Atom as my editor and it keeps giving me an error that it can't find the graphql module in all source files that import it. Any ideas?

@@ -0,0 +1,16 @@
{
"lerna": "2.0.0-beta.30",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is using a beta version the best option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that Im using features that seems to require version 2..
Looks like the rest of the big projects that uses lerna are using beta as well

"version": "0.3.2",
"changelog": {
"repo": "apollostack/apollo-server",
"labels": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the purpose of these labels. They seem odd to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are github issue/PR labels which @helfer needs to create then define there.
Later on it is used for creating changelog between versions

Copy link
Contributor

Choose a reason for hiding this comment

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

@DxCx I might be able to create the labels. Is creating a changelog a feature of lerna?

@@ -8,9 +8,15 @@
"noImplicitAny": false,
"rootDir": "./src",
"outDir": "./dist",
"allowSyntheticDefaultImports": true,
"allowSyntheticDefaultImports": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

why change this to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because its creating arbitrary ///<reference ... in generated d.ts

@nnance
Copy link
Contributor

nnance commented Oct 9, 2016

@DxCx @helfer I have confirmed that with a couple of pretty simple changes we can now remove the use babel from the project. Should we include those changes in this PR or a new PR based on this branch?

@DxCx
Copy link
Contributor Author

DxCx commented Oct 9, 2016

Yeah that was my next goal,
I wanted this to be merged (but yet to be released) then i planned on removing babel.
If you want you can take it but i think it should be done step by step..

@DxCx
Copy link
Contributor Author

DxCx commented Oct 9, 2016

@nnance regarding atom errors
Maybe it parses package.json and not node_modules?
Please make sure that it's there on node_modules

It is peerDependancy for packages that ises it, and global devDep for all the packages
(Want to make sure there is a single version installed)

@stubailo
Copy link
Contributor

Are we still blocked on something for this PR?

@stubailo stubailo mentioned this pull request Oct 20, 2016
@DxCx
Copy link
Contributor Author

DxCx commented Oct 20, 2016

yeah @stubailo i'm updaing the TODOs every time status changes.
right now as you can see we need some of @helfer 's attention :)

@helfer
Copy link
Contributor

helfer commented Oct 20, 2016

@stubailo or @nnance can one take over the items in the todo list currently assigned to me?

@stubailo stubailo mentioned this pull request Oct 22, 2016
4 tasks
@stubailo stubailo merged commit 8df3383 into apollographql:master Oct 23, 2016
@DxCx DxCx deleted the monorepo#138 branch December 1, 2016 12:57
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Looking for a new name! Convert to monorepo
5 participants