Skip to content

Fix crash from attempting to access non-existent dependent key in no-tracked-property-from-args rule #1735

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

Merged

Conversation

joancc
Copy link
Contributor

@joancc joancc commented Jan 4, 2023

Fixes #1734

  1. Open to initial feedback for a better route if this is not desirable. I can add some checks to the no-tracked-properties rule, but figure the current logic should be supported without failing.
const hasThisArgsValue =
    node.value &&
    startsWithThisExpression(node.value) &&
    nodeToDependentKey(node.value, context).split('.')[0] === 'args';
  1. If this path sounds good as a solution, should we be returning the empty string (current) or method name instead?

@bmish bmish added the bug label Jan 14, 2023
}

// Looks like: this.someMethod()
return '';
Copy link
Member

Choose a reason for hiding this comment

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

Let's use undefined to indicate there's no dependent key.

Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

I think my original goal with nodeToDependentKey() was for it to only be called when we knew there would be a dependent key. But since it sounds like it's a hassle for the caller to check for that, your approach of simply bailing out without causing a failure seems reasonable.

@bmish
Copy link
Member

bmish commented Jan 14, 2023

Since it sounds like this fixes a bug in a rule, can you update the PR title to mention that?

@joancc
Copy link
Contributor Author

joancc commented Jan 14, 2023

Thanks for the feedback. Updated to return undefined and added a check on the original rule no-tracked-property-from-args. Wanted to avoid it by returning the empty string, but that's probably misleading.

@joancc joancc changed the title Update nodeToDependentKey to handle nodes of type this.method() Fix no-tracked-property-from-args crash from attempting to access non-existent dependent key Jan 14, 2023
Comment on lines 19 to 20
nodeToDependentKey(node.value, context) &&
nodeToDependentKey(node.value, context).split('.')[0] === 'args';
Copy link
Member

Choose a reason for hiding this comment

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

Having to calculate the dependent key twice isn't ideal. Can't we use optional chaining here?

Suggested change
nodeToDependentKey(node.value, context) &&
nodeToDependentKey(node.value, context).split('.')[0] === 'args';
nodeToDependentKey(node.value, context)?.split('.')[0] === 'args';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point!

Copy link
Member

@bmish bmish 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!

@bmish bmish changed the title Fix no-tracked-property-from-args crash from attempting to access non-existent dependent key Fix crash from attempting to access non-existent dependent key in no-tracked-property-from-args rule Jan 15, 2023
@bmish bmish merged commit 7e81783 into ember-cli:master Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nodeToDependentKey error when passing this.someMethod()
2 participants