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 crash with no-tracked-properties-from-args rule #1712

Merged
merged 4 commits into from Dec 21, 2022

Conversation

joancc
Copy link
Contributor

@joancc joancc commented Dec 20, 2022

Fixes #1711

node.value.object?.type === 'MemberExpression' &&
node.value.object?.object.type === 'ThisExpression' &&
node.value.object?.property.name === 'args';
node.value?.object?.type === 'MemberExpression' &&
Copy link
Member

@bmish bmish Dec 20, 2022

Choose a reason for hiding this comment

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

Normally, it's better practice to be checking the type of the nodes we're dealing with, instead of just checking that various properties exist. Can we check the type on node and node.value before attempting to use them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some really useful helpers and examples already built-ini I just needed to track down, thanks!

@bmish bmish added the Bug label Dec 20, 2022
@bmish
Copy link
Member

bmish commented Dec 20, 2022

Looks like you need to rebase on master, lots of extra commits.

@bmish bmish changed the title Fixes 1711 - Add optional chaining to node value checks Fix crash with no-tracked-properties-from-args rule Dec 21, 2022
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.

Thanks!

It's always a good idea test a new rule on an actual codebase.

@bmish bmish merged commit 389234a into ember-cli:master Dec 21, 2022
rtablada pushed a commit to rtablada/eslint-plugin-ember that referenced this pull request Dec 30, 2022
Co-authored-by: Joan Cejudo <joan@Joans-MacBook-Pro.local>
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
Fixes ember-cli#1711
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.

no-tracked-properties-from-args blows up
2 participants