Monorepo support #35
Closed
Monorepo support #35
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Babel output looks like this Diff lookes like this: https://pastebin.com/P0QSxFcP |
|
Thanks! |
hiroppy
reviewed
Feb 25, 2018
src/index.js
Outdated
| @@ -57,7 +57,11 @@ async function updatePackageJSON(pkg) { | |||
| } | |||
| } | |||
|
|
|||
| const flowConfigs = await globby(['**/.flowconfig', '!**/node_modules/**']); | |||
| const flowConfigs = await globby(['.flowconfig', '!**/node_modules/**'], { | |||
There was a problem hiding this comment.
Is it necessary to change from **/.flowconfig to .flowconfig?
There was a problem hiding this comment.
In the react repo it found the .flowconfig in the root from every package and added flow-type dependency in every package.json even though there is only one .flowconfig in the root.
But as I said I don't have experience with flow yet, maybe this is what should happen?
There was a problem hiding this comment.
Flow takes the closest .flowconfig file it can process (by traversing up the tree) and usually this config is in the root of the project. Also, Flow doesn't traverse for configs down the tree (and thus doesn't merge existing configs). Provided that the command will be run from project's root, this change is necessary.
There was a problem hiding this comment.
Now I am not sure anymore @thymikee
What does this line? It looks for a .flowconfig to figure out if this is a flow project. Then later on based on this check it adds the @babel/preset-flow to the package.json in https://github.com/babel/babel-upgrade/blob/master/src/upgradeDeps.js#L57-L61
This is executed for each package.json it finds, so for each package in a monorepo.
Before my change it will find a .flowconfig in the root and add the dependency to each package. With my change it will only add the dependency to the root package.json and each package that brings it own .flowconfig.
What I do not know: Do the packages in the monorepo need this dependency or is it enough to have this in the root package.json?
There was a problem hiding this comment.
With my change it will only add the dependency to the root package.json and each package that brings it own .flowconfig.
That looks like a proper behavior to me. Usually (but maybe not always), projects in a monorepo use Flow from root and don't contain their own .flowconfig. If some of them do, that would imply that Flow should be run directly from these projects instead from the root.
I'd say – if you find .flowconfig in any package in the monorepo, that package should have its own @babel/preset-flow.
There was a problem hiding this comment.
Thanks for clarifying this! Now I feel more confident with my changes.
|
I still would like someone with flow experience to check if flow repos are handled correct. |
|
@Raigen mind rebasing? I moved the flowconfig detection up a level so that both upgradeDeps and upgradeConfig could handle adding flow-preset properly. |
Find all existings of .babelrc and package.json not in node_modules and update them. Parameterize writePackageJSON with a relative package.json path from globby.
|
I did the rebase and adjustments to my changes. |
|
Ok cool; the thing we don't have are some fixture tests? I know you said you tested it in some repos but we need some tests to prevent regression |
|
And based on your diff with Babel, maybe we need to add an option (doesn't have to be this PR) that can exclude folders or use a glob to ignore? |
|
@Raigen is anything else pending in this PR except fixture tests? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Find all existings of .babelrc and package.json not in node_modules
and update them.
Parameterize writePackageJSON with a relative package.json path
from globby.
Improve log output for babel config
Look for .flowconfig in each package.json directory only
I think this needs some more testing with different projects. I ran it on babel and react, which also gave me a hint to adjust the .flowconfig lookup which I am not happy with yet. But I have no time today to invest further and I also do not have experience with flow, so maybe someone else can try it out.