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

fix(eslint-plugin-template): [no-call-expression] False negative with the new control flow syntax #1677

Merged
merged 7 commits into from
Jan 7, 2024

Conversation

Res42
Copy link
Contributor

@Res42 Res42 commented Jan 6, 2024

Closes #1672

The basic solution was to add more KEYS special cases for preprocessNode.

prefetchTriggers and triggers are simple object dictionaries in @defers AST so I also added Object: ['when'] to the KEYS.


I'll include some ASTs below:

@if

@if (testing1()) {
  <div [id]="testing3()"></div>
} @else if (testing2()) {
  <div [id]="testing4()"></div>
} @else {
  <div [id]="testing5()"></div>
}

Generated AST:

{
  type: 'IfBlock',
  branches: [
    {
      type: 'IfBlockBranch',
      children: [/* ... */], // need to preprocess this too
      expression:  {
        // testing1()
        /* MISSING */ type: 'ASTWithSource',
        ast: {
          // This is the match for the rule
          /* MISSING */ type: 'Call'
        }
      }
    },
    {
      type: 'IfBlockBranch',
      children: [/* ... */], // need to preprocess this too
      expression:  {
        // testing2()
        /* MISSING */ type: 'ASTWithSource',
        ast: {
          // This is the match for the rule
          /* MISSING */ type: 'Call'
        }
      }
    },
    {
      type: 'IfBlockBranch',
      children: [/* ... */], // need to preprocess this too
      expression:  null
    }
  ]
}

Note that the @else block is represented with an 'IfBlockBranch' with expression: null so the code should skip missing KEY properties in the preprocessNode.

@switch

@switch (testing1()) {
  @case(testing2()) {
    <div [id]="testing3()"></div>
  }
  @default {
    <div [id]="testing4()"></div>
  }
}

Generated AST:

{
  type: 'SwitchBlock',
  expression:  {
    // testing1()
    /* MISSING */ type: 'ASTWithSource',
    ast: {
      // This is the match for the rule
      /* MISSING */ type: 'Call'
    }
  },
  cases: [
    {
      type: 'SwitchBlockCase',
      children: [/* ... */], // need to preprocess this too
      expression:  {
        // testing2()
        /* MISSING */ type: 'ASTWithSource',
        ast: {
          // This is the match for the rule
          /* MISSING */ type: 'Call'
        }
      }
    },
    {
      type: 'SwitchBlockCase',
      children: [/* ... */], // need to preprocess this too
      expression:  null
    }
  ]
}

Note that the @default block is represented with an 'SwitchBlockCase' with expression: null so the code should skip missing KEY properties in the preprocessNode here too.

@for

@for (test of testing1(); track test.testing2()) {
  <div [id]="testing3()"></div>
} @empty {
  <div [id]="testing4()"></div>
}

Generated AST:

{
  type: 'ForLoopBlock',
  children: [/* ... */], // need to preprocess this too
  empty: {
    children: [/* ... */], // need to preprocess this too
    /* MISSING */ type: 'ForLoopBlockEmpty'
  },
  expression: {
    // testing1()
    /* MISSING */ type: 'ASTWithSource',
    ast: {
      // This is the match for the rule
      /* MISSING */ type: 'Call'
    }
  },
  trackBy: {
    // test.testing2()
    /* MISSING */type: 'ASTWithSource',
    ast: {
      // This is the match for the rule
      /* MISSING */ type: 'Call'
    }
  }
}

Note that the @empty block can be omitted, so the empty property can be null so the code should skip missing KEY properties in the preprocessNode here too.

@defer

As I see it work has already been done with DeferredBlock so just some modifications are needed.

Basic usage:

@defer {
  <div [id]="testing1()"></div>
} @error {
  <div [id]="testing2()"></div>
} @loading {
  <div [id]="testing3()"></div>
} @placeholder {
  <div [id]="testing4()"></div>
}
{
  type: 'DeferredBlock',
  children: [/* ... */], // need to preprocess this too
  error: {
    // need to preprocess this too
    children: [/* ... */], // need to preprocess this too
  },
  loading: {
    // need to preprocess this too
    children: [/* ... */], // need to preprocess this too
  },
  placeholder:{
    // need to preprocess this too
    children: [/* ... */], // need to preprocess this too
  },
  prefetchTriggers: {},
  triggers: {}
}

The rule should not error on viewport(ref) and other configs that seems like function calls but are built-ins.

@defer (on viewport(ref)) {
  <div [id]="testing()"></div>
}

Generated AST:

{
  type: 'DeferredBlock',
  children: [/* ... */], // need to preprocess this too
  error: null,
  loading: null,
  placeholder: null,
  prefetchTriggers: {},
  triggers: {
    viewport: {
      // no need to do anything here
      /* ... */
    }
  }
}

But it should error on when and prefetch when.

@defer (when testing1(); prefetch when testing2()) {
  <div [id]="testing2()"></div>
}

Generated AST:

{
  type: 'DeferredBlock',
  children: [/* ... */], // need to preprocess this too
  error: null,
  loading: null,
  placeholder: null,
  prefetchTriggers: {
    when: {
      /* MISSING */ type: 'BoundDeferredTrigger',
      value: {
        // testing2()
        /* MISSING */ type: 'Call'
      }
    }
  },
  triggers: {
    when: {
      /* MISSING */ type: 'BoundDeferredTrigger',
      value: {
        // testing1()
        /* MISSING */ type: 'Call'
      }
    }
  }
}

Note that the @error, @loading, @placeholder blocks can be omitted, so the empty, loading, placeholder properties can be null so the code should skip missing KEY properties in the preprocessNode here too.

@Res42 Res42 changed the title [no-call-expression] False negative with the new control flow syntax fix(eslint-plugin-template): [no-call-expression] False negative with the new control flow syntax Jan 6, 2024
@JamesHenry
Copy link
Member

Thank you @Res42 I'm afraid I processed PRs in order, and so an existing PR that was just merged overlapped with this a lot #1656

Please can you reconcile your work against it and see what still needs to be updated/added?

Copy link
Member

@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.

See comment above

@Res42
Copy link
Contributor Author

Res42 commented Jan 7, 2024

Done.

Copy link

nx-cloud bot commented Jan 7, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b60fd4d. 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.

Copy link
Member

@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.

Thank you @Res42!

@JamesHenry JamesHenry merged commit 7a1a42c into angular-eslint:main Jan 7, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[no-call-expression] False negative with the new control flow syntax
2 participants