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): accept various task runner options from root of nx.json #19243

Conversation

AgentEnder
Copy link
Member

Current Behavior

tasksRunnerOptions is a required property for most workspaces, even though they only have 1 runner.

Expected Behavior

This PR simplifies the configuration by allowing most properties to instead be read from the root of the nx.json. One property which changes notably is that cacheableOperations's default value is inferred from the value of cache: true in targetDefaults.

This PR also lays the groundwork for allowing specification of cache: true in a project.json file or similar, but it is hidden behind an env variable until other runners (notably nx cloud) support the property.

Related Issue(s)

Fixes #

@vercel
Copy link

vercel bot commented Sep 19, 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 Oct 5, 2023 2:33pm

* If specified Nx will use nx-cloud by default with the given token.
* To use a different runner that accepts an access token, define it in {@link tasksRunnerOptions}
*/
accessToken?: string;
Copy link
Collaborator

@JamesHenry JamesHenry Sep 20, 2023

Choose a reason for hiding this comment

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

What do you think about nxCloudAccessToken or cloudAccessToken instead?

I don't think accessToken by itself at the root is self-documenting enough for someone who isn't intimately familiar with our ecosystem

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to chat about it, I think its fine though as we will update the schemas to have intellisense and the docs will also show it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm okay with nxCloudAccessToken but I'll defer to @vsavkin .

Copy link
Member Author

Choose a reason for hiding this comment

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

Victor is as well. Changing it.

@AgentEnder AgentEnder force-pushed the feat/accept-task-runner-options-from-root-level branch from 03c49ac to 0b39499 Compare September 20, 2023 20:31
@AgentEnder AgentEnder marked this pull request as ready for review September 20, 2023 22:00
@AgentEnder AgentEnder requested review from a team as code owners September 20, 2023 22:00
packages/nx/src/daemon/client/client.ts Show resolved Hide resolved
packages/nx/src/tasks-runner/run-command.ts Outdated Show resolved Hide resolved
packages/nx/src/tasks-runner/utils.ts Show resolved Hide resolved
packages/nx/src/utils/nx-cloud-utils.ts Outdated Show resolved Hide resolved
packages/nx/src/config/nx-json.ts Show resolved Hide resolved
@AgentEnder AgentEnder force-pushed the feat/accept-task-runner-options-from-root-level branch from 0b39499 to d7c234c Compare September 23, 2023 03:23
@AgentEnder AgentEnder force-pushed the feat/accept-task-runner-options-from-root-level branch from d7c234c to 6c20939 Compare September 27, 2023 14:11
* If specified Nx will use nx-cloud by default with the given token.
* To use a different runner that accepts an access token, define it in {@link tasksRunnerOptions}
*/
accessToken?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm okay with nxCloudAccessToken but I'll defer to @vsavkin .

parallel?: number;

/**
* Changes the default location of the cache directory.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Changes the default location of the cache directory.
* Changes the directory used by Nx to store its cache.

accessToken?: string;

/**
* Specifies how many tasks are ran in parallel by Nx for the default tasks runner.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Specifies how many tasks are ran in parallel by Nx for the default tasks runner.
* Specifies how many tasks can be run in parallel.

cacheDirectory?: string;

/**
* Allows turning the daemon off if set to false explicitly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Allows turning the daemon off if set to false explicitly.
* Set this to false to disable the daemon

@@ -23,6 +24,15 @@ Target's configuration

## Properties

### cache
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this typing definitino to TargetDefaults for now.

@@ -64,7 +64,9 @@ export class DaemonClient {

enabled() {
if (this._enabled === undefined) {
// TODO: Add migration to move it out of existing configs and remove the ?? here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// TODO: Add migration to move it out of existing configs and remove the ?? here.
// TODO(v18): Add migration to move it out of existing configs and remove the ?? here.

Comment on lines 462 to 471
if (nxJson.accessToken) {
result.accessToken ??= nxJson.accessToken;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ONLY send this in if nx-cloud is used.

@FrozenPandaz FrozenPandaz merged commit 9474841 into nrwl:master Oct 5, 2023
6 checks passed
@github-actions
Copy link

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 Oct 11, 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.

3 participants