-
Notifications
You must be signed in to change notification settings - Fork 214
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
fix: update default branch text to "main" #1143
fix: update default branch text to "main" #1143
Conversation
☁️ Nx Cloud ReportCI ran the following commands for commit b2d3681. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
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.
This should wait until nrwl/nx#7111 is merged and released. Current default behavior with create-nx-workspace
is to have a default branch of master (and I'd argue that most Nx workspaces still use master since the change is somewhat recent)
nx.json
Outdated
@@ -6,7 +6,7 @@ | |||
".eslintrc.json": "*" | |||
}, | |||
"affected": { | |||
"defaultBase": "master" | |||
"defaultBase": "main" |
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.
This affects this repo, not projects using nx console. Since this repo still has master for main branch changing this would cause commands ran in this repo to break, without changing the end user experience.
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.
Oh I didn't know that 😓 Thanks for the good look @AgentEnder o7
This is definitely something we will want to merge when the time is right though! Thanks for the PR 😄 |
I appreciate the work in this. It's definitely something on the radar to do :D |
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 you revert the changes in package-scripts.js, and nx.json, this PR should be good to merge. The master
-> main
change in the main nx repo is complete and should handle most concerns.
package-scripts.js
Outdated
'origin-main': `nx affected:${affectedCommand} --base=origin/main --parallel --silent --ci`, | ||
'upstream-main': `nx affected:${affectedCommand} --base=upstream/main --parallel --silent --ci`, |
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.
These are also for this repo, not downstream consumers
package-scripts.js
Outdated
default: 'nx format:write --base=upstream/main', | ||
and: { | ||
lint: { | ||
check: nps.concurrent.nps('format.check', 'lint'), | ||
}, | ||
}, | ||
write: 'nx format:write --base=upstream/master', | ||
check: 'nx format:check --base=upstream/master', | ||
write: 'nx format:write --base=upstream/main', | ||
check: 'nx format:check --base=upstream/main', |
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.
These are also for this repo, not downstream consumers
I'm going to do some cleanup and get this one merged in. |
7c33e14
to
51369f9
Compare
Motivation?
When creating a new workspace,
nx
init a git repository. Sincegit
default branch name policy has changed frommaster
tomain
. This lead commands such asnx affected:build
to crash after a fresh install. All people new to nx will eventually face this issue.What's been done?
Replacing
master
withmain
where needed.Breaking changes?
If running
nx migrate
replaces some of the updated files, then the commands will break for projects havingmaster
as their default base. The solution is to use the--base
flag on such commands.