search

prodo-dev / prodo

Prodo is a React framework to build apps faster.
https://docs.prodo.dev
MIT License
115 stars 5 forks source link

check def exists before accessing node #141

Closed kprimerakis closed 4 years ago

kprimerakis commented 4 years ago

Simple fix for bug. I saw the following error when linting a file that included builtin variables (Date, console etc). Fixed by checking that the definition exists before accessing node in the findDefinition function. 

Occurred while linting /prodo/examples/chess-app/src/App.tsx:6
    at Object.exports.findDefinition (/prodo/packages/eslint-plugin/lib/utils/findDefinition.js:10:60)
    at CallExpression:not(CallExpression > CallExpression.callee) (/prodo/packages/eslint-plugin/lib/rules/require-dispatched-action-is-called.js:40:58)
    at listeners.(anonymous function).forEach.listener (/prodo/node_modules/eslint/lib/linter/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/prodo/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector
...
...
kprimerakis commented 4 years ago

I tried. Thing is, I don't know how to test for this. Tests do include console and I couldn't get them to fail.

In the tests, the console variable isn't resolved, in my example it is. So I can't get the tests to fail because the resolved condition in line 15 fails. I have no idea why this happens.

tdawes commented 4 years ago

This test for no-state-access throws an exception on master:

Date.now();

I imagine console works differently for some reason.

tdawes commented 4 years ago

Looks like it is different for console and Date:

Screenshot 2019-10-02 at 17 28 46

Screenshot 2019-10-02 at 17 28 35

notice that the reference for console has resolved: null, but Date doesn't.

kprimerakis commented 4 years ago

Yep, date fails in the tests.

We only have regression tests currently. Do we need a unit test for utils? I was just going to add a Date variable in one of the existing regression tests, we happy with that?