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 create nodes v2 for batch processing config files #26250

Merged
merged 3 commits into from
May 30, 2024

Conversation

AgentEnder
Copy link
Member

@AgentEnder AgentEnder commented May 29, 2024

Current Behavior

CreateNodes as an API has done wonders in terms of the flexibility it has added for plugins to be able to define new attributes about projects, and even full projects. We've done a lot with it, and love where its going, but we've also noticed some pain points.

  • Some plugins want to do some work before / after all of their create nodes functions have ran. For example, reading / writing to a cache.
  • If a plugin wants to have a cache, multiple instances of that plugin end up reading / writing to the same cache file if its specified in multiple entries in nx.json's plugin array even if it has different options.

This can be illustrated by looking at the plugins we have written for project crystal. Several of them use a module-scoped cache, s.t. it is initialized before all of the plugins run and then write the cache in the create dependencies because we know that all nodes are present at that point. These plugins shouldn't even really have a create dependencies section as most don't identify any deps.

Additionally, when we turn on plugin isolation things get wonky. Since the multiple instances of the plugin are now on their own threads and each were writing to the same files, they would try to read and write to the same file at the same time causing crashes. The logical way to solve this was to have them write to separate cache files, but we didn't have a way to provide a unique identifier to each plugin instance given the cache stuff was happening at the module scope.

Another pain point is that some plugins care about all of the config files that were found by their configuration file pattern. This was evident in our ESLint plugin, and we initially worked around the limitation by adding all found files into the plugin's CreateNodesContext. While that worked, it added some extra work that needed to be done in the plugin since each call of createNodes wasn't aware of the other calls and we had to do some deduplication work which shouldn't have actually been needed.

Expected Behavior

https://www.loom.com/share/1c0e9ed815934e3cbb762232f64fe0dd?sid=c17d7d62-9999-4b94-a897-ef9e7e1389d6

We started down the path of providing some extra hooks to handle setup and teardown work for createNodes. This was explored in #26163. This path would have worked in some cases, but after sleeping on it @FrozenPandaz and I arrived at a different solution that while more painful for existing plugins we think should have better long term viability.

We are iterating on the CreateNodes API and providing a V2. V2 will replace the current API over the course of Nx 20 and Nx 21. Docs on how plugin authors should maintain compatibility with existing versions will follow. The timeline for transitioning looks something like this:

Nx 19.2+ Both original CreateNodes and CreateNodesV2 are supported. Nx will only invoke CreateNodesV2 if it is present.
Nx 20.X The CreateNodesV2 will be the only supported API. This typing will still exist, but be identical to CreateNodesV2. Nx will not invoke the original plugin.createNodes callback. This should give plugin authors a window to transition their API.
Nx 21.X The CreateNodesV2 typing will be removed, as it has replaced CreateNodes.

The new API is similar to a batch implementation. Your CreateNodesV2 function will receive a list of all matching files as its first argument. It will return a list of tuples, in the shape Array<[file: string, result: CreateNodesResult]>. We recognize that for some plugins this may feel a bit awkward, but it does solve the issues we noted.

Problem Workaround with current API Solution path
pre/post all create nodes hooks module scope init, create deps cleanup beginning + end of v2 function
file read/write collisions include process.pid in path? include hash of plugin options in path
functions need to be aware of other files context.configFiles first argument = all config files

createNodesFromFiles

For both ease of transitioning as well as ease of writing, we are exporting a utility function called createNodesFromFiles which accepts the v1 API as a callback along with the list of files, options, and context. This function is not JUST a conversion utility and will remain stable API for the foreseeable future. Having a function to process a single config file is still an easier function to write than a function to process all config files. This function allows you to write the simple function to process a single file and pass it into a function which will use it to create the right results for all config files.

plugin.ts

import { createNodesFromFiles, ... } from '@nx/devkit';

// Keep this exported for older versions of Nx.
export const createNodes: CreateNodes = ['**/config.json', processSingleConfigFile];

// Use `createNodesFromFiles` to reuse the logic from `processSingleConfigFile`
export const createNodesV2: CreateNodesV2 = ['**/config.json', async (configFiles, options, context) => {
  // Do some stuff before
  
  try {
    return await createNodesFromFiles(processSingleConfigFile, configFiles, options, context);
  } finally {
    // Do some stuff after
  }
}

Related Issue(s)

Fixes #

Copy link

vercel bot commented May 29, 2024

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

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview May 30, 2024 4:57pm

@@ -167,7 +167,7 @@ export function isProjectWithNoNameError(
export class ProjectConfigurationsError extends Error {
constructor(
public readonly errors: Array<
| MergeNodesError

This comment was marked as resolved.

@@ -191,19 +191,21 @@ export function isProjectConfigurationsError(
}

export class CreateNodesError extends Error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used?

@@ -215,9 +217,8 @@ export class CreateNodesError extends Error {

export class AggregateCreateNodesError extends Error {

This comment was marked as resolved.

@@ -47,16 +60,29 @@ export interface CreateNodesResult {
* A map of external node name -> external node. External nodes do not have a root, so the key is their name.
*/
externalNodes?: Record<string, ProjectGraphExternalNode>;

errors?: Error[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

}

/**
* A pair of file patterns and {@link CreateNodesFunction}
*
* @deprecated Use {@link CreateNodesV2} instead. In Nx 20 support for calling createNodes with a single file for the first argument will be removed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nx 19 - We call v2 (new) or v1 (old)
Nx 20 - We only call v2 (new), plugin owners should switch to having both v2 (new) and v1 be the same (new). This type will change to v2
Nx 21 - We go back to calling only createNodes (new signature)

return gradleReportCache;
}

let gradleCurrentConfigHash: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep this with the instantiation of the cache.

@@ -25,9 +26,25 @@ export function invalidateGradleReportCache() {
gradleReportCache = undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this.

@@ -84,10 +120,21 @@ export const createNodes: CreateNodes<GradlePluginOptions> = [
[projectRoot]: project,
},
};
};

export const createNodes: CreateNodes<GradlePluginOptions> = [

This comment was marked as resolved.

@AgentEnder AgentEnder requested a review from FrozenPandaz May 30, 2024 17:29
@AgentEnder AgentEnder marked this pull request as ready for review May 30, 2024 17:52
@AgentEnder AgentEnder requested review from a team and xiongemi as code owners May 30, 2024 17:52
@AgentEnder AgentEnder requested a review from leosvelperez May 30, 2024 17:52
@AgentEnder AgentEnder merged commit a5682d1 into master May 30, 2024
6 checks passed
@AgentEnder AgentEnder deleted the create-nodes-v2 branch May 30, 2024 19:29
Copy link

github-actions bot commented Jun 5, 2024

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 Jun 5, 2024
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.

2 participants