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

feat(core): add monorepo generator to convert from standalone projects #18245

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

jaysoo
Copy link
Member

@jaysoo jaysoo commented Jul 21, 2023

This PR adds a new @nx/workspace:monorepo generator that converts standalone projects to a monorepo.

For example:

npx create-nx-workspace react-app
cd react-app
nx g monorepo

Then users will see:

.
├── apps
│   ├── e2e
│   │   ├── cypress.config.ts
│   │   ├── project.json
│   │   ├── src
│   │   │   ├── ...
│   │   │   └── support
│   │   └── tsconfig.json
│   └── react2
│       ├── index.html
│       ├── project.json
│       ├── public
│       │   └── favicon.ico
│       ├── src
│       │   ├── app
│       │   ├── assets
│       │   ├── main.tsx
│       │   └── styles.css
│       ├── tsconfig.app.json
│       ├── tsconfig.json
│       ├── tsconfig.spec.json
│       └── vite.config.ts
├── nx.json
├── README.md
├── package.json
└── tsconfig.base.json
generator-monorepo.mp4

Test Cases

  1. Convert a clean standalone project (i.e. one from create-nx-workspace without any changes). Test with React, Next.js, Angular, Node servers, TS.
  2. Convert a standalone project with libs already added.
  3. Convert a standalone project with other apps added.
  4. Convert a standalone project with some apps and libs already in apps and libs directories.
  5. Convert a standalone TS project 'my-lib', but packages/my-lib or libs/my-lib already exists.
  6. Convert a monorepo (idempotency)

Notes

  • It's hard to move all root project files to their new location under appssince there are workspace files such as nx.json, or even custom files that users create that may or may not belong to the root project. The safest way to handle it is to whitelist the root files we do move, and leave the rest for users to handle.
  • The existing e2e project is problematic since it is not easy to replace "e2e" string in configuration files with apps/e2e without messing options like "target": "e2e" that should be left untouched. We may need to handle these cases specifically.

@vercel
Copy link

vercel bot commented Jul 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 28, 2023 4:59pm

Copy link
Collaborator

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Copying over my slack reply just so it doesn't get lost:

Nice! We have a precedent for conversion generators starting with convert to make it clear it’s mutating existing stuff as its focus, so maybe convert-to-monorepo is preferable?

As Victor notes it’s going to be run very rarely so arguably clarity can be weighted above brevity because it’s not an ongoing source of friction to type?

I would worry nx generate @nx/workspace:monorepo could easily be confused with create-nx-workspace

Comment on lines 18 to 16
const nxJson = readNxJson(tree);
nxJson.workspaceLayout = {
// normalize paths without trailing slash
appsDir: joinPathFragments(options.appsDir),
libsDir: joinPathFragments(options.libsDir),
};

updateNxJson(tree, nxJson);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just add the directories and it will have the same effect. Do not write to nx.json.

This can move after the move.

  1. If we move an app, we also have to add the libs dir that the user wants
  2. If we move a lib, we don't actually have to do anything because moving the project will create the libs dir

Comment on lines 15 to 20
"appsDir": {
"type": "string",
"description": "The directory where apps are placed.",
"default": "apps",
"x-prompt": "Where do you want to place your apps?"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's always pick apps if the root project is an app. Do not prompt.

Comment on lines 21 to 27
"libsDir": {
"type": "string",
"alias": "packagesDir",
"description": "The directory where libs/packages are placed.",
"default": "libs",
"x-prompt": "Where do you want to place your libs/packages?"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Constrain this to ['libs', 'packages']

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out, we're restricted to apps-libs vs packages anyway, so removing all the prompts. i.e. you cannot have apps-packages.

maybeExtractEslintConfigIfRootProject(tree, project);
await moveGenerator(tree, {
projectName: project.name,
destination: project.name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pass destinationRelativeToRoot so that the destination is taken as is.

This turns into join(options.appsDir | options.libsDir, project.name)

Comment on lines 15 to 16
mayebExtractTsConfigBase(tree);
await maybeExtractJestConfigBase(tree);
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of the extract, should go into move. Regardless of if we are moving from a monorepo or not, if a user moves the root app, we should do this stuff.

This could be refactored more later.

const relativeFromOriginalSource = relative(project.root, file);
const newFilePath = join(
schema.relativeToRootDestination,
relativeFromOriginalSource
);

// Prevents moving the same file infinitely for root projects
if (seen.has(file)) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't needed anymore after the whitelist.

.relative(path.join(workspaceRoot, project.root), workspaceRoot)
.split(path.sep)
.join('/');
const oldRelativeRoot =
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be reverted.

if (file === '.eslintrc.json') {
continue;
}
if (!shouldUpdate(file)) continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go back to how it was.

Comment on lines +156 to +158
expect(tree.exists('package.json')).toBeTruthy();
expect(tree.exists('README.md')).toBeTruthy();
expect(tree.exists('.gitignore')).toBeTruthy();
expect(tree.exists('other-lib/index.ts')).toBeTruthy();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should test that root config files are made

formatFiles,
ProjectGraph,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unneeded imports

Copy link
Contributor

@barbados-clemens barbados-clemens left a comment

Choose a reason for hiding this comment

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

just a question

packages/jest/src/generators/init/init.ts Show resolved Hide resolved
@@ -2842,6 +2842,15 @@
"path": "workspace/generators/remove",
"type": "generator"
},
{
"description": "Convert a Nx project to a monorepo.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ndcunningham Can you make this change in a follow up PR please?

@FrozenPandaz FrozenPandaz merged commit dcefa4a into nrwl:master Jul 31, 2023
15 checks passed
@github-actions
Copy link

github-actions bot commented Aug 6, 2023

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 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.

None yet

5 participants