-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
cleanup(misc): clean up init generators #21088
cleanup(misc): clean up init generators #21088
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 09785ce. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 6 targets
Sent with 💌 from NxCloud. |
5532c74
to
8522264
Compare
8522264
to
ce6c153
Compare
70692fa
to
31ab88e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small comments. I'm testing this out locally and then I'll approve.
…generator from other plugins
31ab88e
to
09785ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it locally:
- generate new workspace
- add react app
- add vue app
- add nuxt app
- add storybook
All works as expected.
Added a question, the rest LGTM. |
? addDependenciesToPackageJson( | ||
const tasks: GeneratorCallback[] = []; | ||
if (!options.skipPackageJson) { | ||
tasks.push(removeDependenciesFromPackageJson(tree, ['@nx/eslint'], [])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the purpose of removing this dependency before re-adding it was to avoid version clash if the same package is installed in dependencies
and devDependencies
then wouldn't we want to do the same with the other packages listed below - eslint
, @nx/eslint-plugin
, @typescript-eslint/*
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there's follow-up work to use the updated addDependenciesToPackageJson
, the util PR is pending merge: #21123
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that was the reason, but I'm unsure. I just left the existing behavior for this. IMHO, we shouldn't do it for any. AFAIK, the addDependenciesToPackageJson
function handles this though maybe differently. I haven't checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one minor comment but otherwise looks good to me.
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. |
This PR simplifies the tasks performed by
init
generator in the following way:init
generatorsinit
generatorsinit
generatorsThe
init
generators were always internal and meant to be called by other generators to ensure the base dependencies and configuration were set up. This is changing. While they'll continue to be internal, soon they could also be invoked when runningnx add
andnx init
. In those scenarios, we only need to initialize the minimum required for the plugin to work. We wouldn't be generating anything just yet and can't make assumptions and install/configure things that might not be needed by the workspace.Current Behavior
Expected Behavior
Related Issue(s)
Fixes #