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.

Sign up for GitHub

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

Jump to bottom

Fix no-array-prototype extensions undefined error from trying to access callee from non-CallExpression #1788

Merged
merged 3 commits into from
Mar 13, 2023
Merged

Fix no-array-prototype extensions undefined error from trying to access callee from non-CallExpression #1788

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0a4c71
fix undefined error on no-array-prototype extensions
canrozanes Feb 26, 2023
f3d2703
replace type.callee with type === 'CallExoression'
canrozanes Mar 8, 2023
161cc08
Merge branch 'master' into fix-ember-prototype-extension-error
lin-ll Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/rules/no-array-prototype-extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,7 @@ module.exports = {
if (
node.type === 'CallExpression' &&
importedEmberArrayName &&
nodeInitializedTo.callee &&
Copy link
Member

Choose a reason for hiding this comment

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

Better to check that nodeInitializedTo.type === 'CallExpression'.

Also need a test of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lack of a test case is the reason I left this PR as a draft. I'm not sure how to reproduce this in a test.

Would I import from the same path like import { SOMETHING } from '@ember/array' and make sure it doesn't have a callee?

What would that even look like?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think maybe this could reproduce the error?

import { A } from '@ember/array';

this.foo = A();
this.foo.toArray();

I think in this case, node.InitializedTo.type is no longer a CallExpression but rather a MemberExpression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lin-ll when I add your test case,

without the fix above, I get the following error:

 TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'Node'
        |     property 'object' -> object with constructor 'Node'
        --- property 'parent' closes the circle
        at stringify (<anonymous>)

with the fix above, I get a legitimate linting error:

Message:
      Should have no errors but had 1: [
      {
        ruleId: 'no-array-prototype-extensions',
        severity: 1,
        message: "Don't use Ember's array prototype extensions",
        line: 5,
        column: 5,
        nodeType: 'CallExpression',
        messageId: 'main',
        endLine: 5,
        endColumn: 23,
        fix: { range: [Array], text: '[...this.foo]' }
      }
    ]

The linting error is a false positive. This rule probably should allow your test case but it currently doesn't.

So either way, it's not a valid test case for this fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm really? When I run that example (yarn test tests/lib/rules/no-array-prototype-extensions.js), I get the correct undefined error:

  ● no-array-prototype-extensions › valid › 
    import { A } from '@ember/array';

this.foo = A();
this.foo.toArray();
    

    TypeError: Cannot read property 'type' of undefined
    Occurred while linting <input>:5
    Rule: "no-array-prototype-extensions"

      753 |           node.type === 'CallExpression' &&
      754 |           importedEmberArrayName &&
    > 755 |           nodeInitializedTo.callee.type === 'Identifier' &&
          |                                    ^
      756 |           importedEmberArrayName === nodeInitializedTo.callee.name
      757 |         ) {
      758 |           return;

Agreed that with your fix, it doesn't fix the false positive -- but it does seem like a valid way of reproducing this error

nodeInitializedTo.callee.type === 'Identifier' &&
importedEmberArrayName === nodeInitializedTo.callee.name
) {
Expand Down