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

void-dom-elements-no-children throws a TypeError when installed from the current master branch. #1101

Closed
phyllisstein opened this issue Mar 5, 2017 · 12 comments

Comments

@phyllisstein
Copy link

Hey there! I hate to be a bother, but I just tried installing this plugin from GitHub, overeager to see if JSX indentation had been improved, and ran into a little trouble linting one of my source files. The void-dom-elements-no-children rule is throwing a TypeError on line 107:

Cannot read property 'value' of undefined
TypeError: Cannot read property 'value' of undefined
    at EventEmitter.CallExpression (/Users/daniel/Repos/Ignota/rupertsberg/node_modules/eslint-plugin-react/lib/rules/void-dom-elements-no-children.js:107:34)
    at emitOne (events.js:101:20)
    at EventEmitter.emit (events.js:191:7)
    at NodeEventGenerator.enterNode (/Users/daniel/Repos/Ignota/rupertsberg/node_modules/eslint/lib/util/node-event-generator.js:39:22)
    at CodePathAnalyzer.enterNode (/Users/daniel/Repos/Ignota/rupertsberg/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:607:23)
    at CommentEventGenerator.enterNode (/Users/daniel/Repos/Ignota/rupertsberg/node_modules/eslint/lib/util/comment-event-generator.js:98:23)
    at Controller.enter (/Users/daniel/Repos/Ignota/rupertsberg/node_modules/eslint/lib/eslint.js:928:36)
    at Controller.__execute (/Users/daniel/Repos/Ignota/rupertsberg/node_modules/estraverse/estraverse.js:397:31)
    at Controller.traverse (/Users/daniel/Repos/Ignota/rupertsberg/node_modules/estraverse/estraverse.js:501:28)
    at Controller.Traverser.controller.traverse (/Users/daniel/Repos/Ignota/rupertsberg/node_modules/eslint/lib/util/traverser.js:36:33)

Logging node.arguments shows that it's an empty array just before the crash, but honest I don't know nearly enough about your lovely linter's deep magic to say any more than that. Please let me know if it'd be helpful to Gist the source file, though, or if I can furnish any other information I've got. Disabling the rule in my ESLint config stops the crash for now.

@ljharb
Copy link
Member

ljharb commented Mar 5, 2017

It would absolutely be helpful to gist the source file it crashes on, thanks!

@phyllisstein
Copy link
Author

Sure thing! Went ahead and dumped the JSX, my .eslintrc.yml, and the error here: https://gist.github.com/phyllisstein/04036a187aa4c2d4195a67cefb4170cb. Let me know if you need anything else!

@ljharb
Copy link
Member

ljharb commented Mar 5, 2017

@phyllisstein
Copy link
Author

So I did a little ham-handed investigation and started deleting lines blindly. Turns out it's the handleClick function? Including any of the lines in the range https://gist.github.com/phyllisstein/04036a187aa4c2d4195a67cefb4170cb#file-button-jsx-L58-L64 causes it to throw, whereas deleting it entirely seems to work around the issue. Heaven knows what that portends though.

@ljharb
Copy link
Member

ljharb commented Mar 5, 2017

Interesting - probably because it's an arrow function class property. If instead (which would be much more performant) it was a true instance method, and you did this.handleClick = this.handleClick.bind(this) in the constructor, I suspect it'd work.

Thanks though, this should be very helpful to fix the bug.

@Hypnosphi
Copy link
Contributor

which would be much more performant

Why exactly? You still create a bound function in constructor, that's roughly what arrow function property compiles to

@ljharb
Copy link
Member

ljharb commented Apr 25, 2017

Yes, in the constructor once, instead of thousands of times once per render.

@timhwang21
Copy link

I thought if the arrow function is a class property, it is also only computed once?

class Foo extends Component {
  myArrowFunc = () => { ... }
}

Din't think @Hypnosphi is talking about arrow functions within render().

@ljharb
Copy link
Member

ljharb commented Apr 25, 2017

Yes, that's true. But at that point the meat of the function isn't in an optimized prototype method, its newly created on every render. Constructor-bound prototype methods are the best approach.

@timhwang21
Copy link

I didn't know that, thanks! Fixes an incorrect belief of mine.

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Apr 25, 2017

class Foo {
  myArrowFunc = () => { ... }
}

compiles to

class Foo {
  constructor() {
    this.myArrowFunc = () => { ... };
  }
}

Excuse me, but I don't see any rendering here. Did you mean "on every constructor call" instead? Then it shouldn't occur too often in a typical react app.

@ljharb
Copy link
Member

ljharb commented Apr 25, 2017

@Hypnosphi yes, you're right that your example isn't about the render path. However, #1101 (comment) still applies.

github-actions bot pushed a commit to capitnflam/eslint-plugin that referenced this issue Mar 16, 2024
# [1.1.0](v1.0.1...v1.1.0) (2024-03-16)

### chore

* **deps-dev:** bump [@types](https://github.com/types)/node from 20.11.27 to 20.11.28 ([#13](#13)) ([738e28c](738e28c))
* **deps:** bump eslint-plugin-react from 7.34.0 to 7.34.1 ([#12](#12)) ([ad9f1b3](ad9f1b3)), closes [#3700](https://github.com/capitnflam/eslint-plugin/issues/3700) [#3701](https://github.com/capitnflam/eslint-plugin/issues/3701) [#3704](https://github.com/capitnflam/eslint-plugin/issues/3704) [#3705](https://github.com/capitnflam/eslint-plugin/issues/3705) [#3707](https://github.com/capitnflam/eslint-plugin/issues/3707) [#3713](https://github.com/capitnflam/eslint-plugin/issues/3713) [#3715](https://github.com/capitnflam/eslint-plugin/issues/3715) [#1000](https://github.com/capitnflam/eslint-plugin/issues/1000) [jsx-eslint/eslint-plugin-react#1000](jsx-eslint/eslint-plugin-react#1000) [#1002](https://github.com/capitnflam/eslint-plugin/issues/1002) [jsx-eslint/eslint-plugin-react#1002](jsx-eslint/eslint-plugin-react#1002) [#1005](https://github.com/capitnflam/eslint-plugin/issues/1005) [jsx-eslint/eslint-plugin-react#1005](jsx-eslint/eslint-plugin-react#1005) [#100](https://github.com/capitnflam/eslint-plugin/issues/100) [jsx-eslint/eslint-plugin-react#100](jsx-eslint/eslint-plugin-react#100) [#1010](https://github.com/capitnflam/eslint-plugin/issues/1010) [jsx-eslint/eslint-plugin-react#1010](jsx-eslint/eslint-plugin-react#1010) [#1013](https://github.com/capitnflam/eslint-plugin/issues/1013) [jsx-eslint/eslint-plugin-react#1013](jsx-eslint/eslint-plugin-react#1013) [#1022](https://github.com/capitnflam/eslint-plugin/issues/1022) [jsx-eslint/eslint-plugin-react#1022](jsx-eslint/eslint-plugin-react#1022) [#1029](https://github.com/capitnflam/eslint-plugin/issues/1029) [jsx-eslint/eslint-plugin-react#1029](jsx-eslint/eslint-plugin-react#1029) [#102](https://github.com/capitnflam/eslint-plugin/issues/102) [jsx-eslint/eslint-plugin-react#102](jsx-eslint/eslint-plugin-react#102) [#1034](https://github.com/capitnflam/eslint-plugin/issues/1034) [jsx-eslint/eslint-plugin-react#1034](jsx-eslint/eslint-plugin-react#1034) [#1038](https://github.com/capitnflam/eslint-plugin/issues/1038) [jsx-eslint/eslint-plugin-react#1038](jsx-eslint/eslint-plugin-react#1038) [#1041](https://github.com/capitnflam/eslint-plugin/issues/1041) [jsx-eslint/eslint-plugin-react#1041](jsx-eslint/eslint-plugin-react#1041) [#1043](https://github.com/capitnflam/eslint-plugin/issues/1043) [jsx-eslint/eslint-plugin-react#1043](jsx-eslint/eslint-plugin-react#1043) [#1046](https://github.com/capitnflam/eslint-plugin/issues/1046) [jsx-eslint/eslint-plugin-react#1046](jsx-eslint/eslint-plugin-react#1046) [#1047](https://github.com/capitnflam/eslint-plugin/issues/1047) [jsx-eslint/eslint-plugin-react#1047](jsx-eslint/eslint-plugin-react#1047) [#1050](https://github.com/capitnflam/eslint-plugin/issues/1050) [jsx-eslint/eslint-plugin-react#1050](jsx-eslint/eslint-plugin-react#1050) [#1053](https://github.com/capitnflam/eslint-plugin/issues/1053) [jsx-eslint/eslint-plugin-react#1053](jsx-eslint/eslint-plugin-react#1053) [#1057](https://github.com/capitnflam/eslint-plugin/issues/1057) [jsx-eslint/eslint-plugin-react#1057](jsx-eslint/eslint-plugin-react#1057) [#105](https://github.com/capitnflam/eslint-plugin/issues/105) [jsx-eslint/eslint-plugin-react#105](jsx-eslint/eslint-plugin-react#105) [#1061](https://github.com/capitnflam/eslint-plugin/issues/1061) [jsx-eslint/eslint-plugin-react#1061](jsx-eslint/eslint-plugin-react#1061) [#1062](https://github.com/capitnflam/eslint-plugin/issues/1062) [jsx-eslint/eslint-plugin-react#1062](jsx-eslint/eslint-plugin-react#1062) [#1070](https://github.com/capitnflam/eslint-plugin/issues/1070) [jsx-eslint/eslint-plugin-react#1070](jsx-eslint/eslint-plugin-react#1070) [#1071](https://github.com/capitnflam/eslint-plugin/issues/1071) [jsx-eslint/eslint-plugin-react#1071](jsx-eslint/eslint-plugin-react#1071) [#1073](https://github.com/capitnflam/eslint-plugin/issues/1073) [jsx-eslint/eslint-plugin-react#1073](jsx-eslint/eslint-plugin-react#1073) [#1076](https://github.com/capitnflam/eslint-plugin/issues/1076) [jsx-eslint/eslint-plugin-react#1076](jsx-eslint/eslint-plugin-react#1076) [#1079](https://github.com/capitnflam/eslint-plugin/issues/1079) [jsx-eslint/eslint-plugin-react#1079](jsx-eslint/eslint-plugin-react#1079) [#1088](https://github.com/capitnflam/eslint-plugin/issues/1088) [jsx-eslint/eslint-plugin-react#1088](jsx-eslint/eslint-plugin-react#1088) [#1098](https://github.com/capitnflam/eslint-plugin/issues/1098) [jsx-eslint/eslint-plugin-react#1098](jsx-eslint/eslint-plugin-react#1098) [#1101](https://github.com/capitnflam/eslint-plugin/issues/1101) [jsx-eslint/eslint-plugin-react#1101](jsx-eslint/eslint-plugin-react#1101) [#1103](https://github.com/capitnflam/eslint-plugin/issues/1103) [jsx-eslint/eslint-plugin-react#1103](jsx-eslint/eslint-plugin-react#1103) [#110](https://github.com/capitnflam/eslint-plugin/issues/110) [jsx-eslint/eslint-plugin-react#110](jsx-eslint/eslint-plugin-react#110) [#1116](https://github.com/capitnflam/eslint-plugin/issues/1116) [jsx-eslint/eslint-plugin-react#1116](jsx-eslint/eslint-plugin-react#1116) [#1117](https://github.com/capitnflam/eslint-plugin/issues/1117) [jsx-eslint/eslint-plugin-react#1117](jsx-eslint/eslint-plugin-react#1117) [#1119](https://github.com/capitnflam/eslint-plugin/issues/1119) [jsx-eslint/eslint-plugin-react#1119](jsx-eslint/eslint-plugin-react#1119) [#1121](https://github.com/capitnflam/eslint-plugin/issues/1121) [jsx-eslint/eslint-plugin-react#1121](jsx-eslint/eslint-plugin-react#1121) [#1122](https://github.com/capitnflam/eslint-plugin/issues/1122) [jsx-eslint/eslint-plugin-react#1122](jsx-eslint/eslint-plugin-react#1122) [#1123](https://github.com/capitnflam/eslint-plugin/issues/1123) [jsx-eslint/eslint-plugin-react#1123](jsx-eslint/eslint-plugin-react#1123) [#3700](https://github.com/capitnflam/eslint-plugin/issues/3700) [#3701](https://github.com/capitnflam/eslint-plugin/issues/3701) [#3704](https://github.com/capitnflam/eslint-plugin/issues/3704) [#3705](https://github.com/capitnflam/eslint-plugin/issues/3705) [#3707](https://github.com/capitnflam/eslint-plugin/issues/3707) [#3713](https://github.com/capitnflam/eslint-plugin/issues/3713) [#3715](https://github.com/capitnflam/eslint-plugin/issues/3715) [#3715](https://github.com/capitnflam/eslint-plugin/issues/3715) [jsx-eslint/eslint-plugin-react#3715](jsx-eslint/eslint-plugin-react#3715) [#3713](https://github.com/capitnflam/eslint-plugin/issues/3713) [jsx-eslint/eslint-plugin-react#3713](jsx-eslint/eslint-plugin-react#3713) [#3707](https://github.com/capitnflam/eslint-plugin/issues/3707) [jsx-eslint/eslint-plugin-react#3707](jsx-eslint/eslint-plugin-react#3707) [#3705](https://github.com/capitnflam/eslint-plugin/issues/3705) [jsx-eslint/eslint-plugin-react#3705](jsx-eslint/eslint-plugin-react#3705) [#3704](https://github.com/capitnflam/eslint-plugin/issues/3704) [jsx-eslint/eslint-plugin-react#3704](jsx-eslint/eslint-plugin-react#3704) [#3701](https://github.com/capitnflam/eslint-plugin/issues/3701) [jsx-eslint/eslint-plugin-react#3701](jsx-eslint/eslint-plugin-react#3701) [#3700](https://github.com/capitnflam/eslint-plugin/issues/3700) [jsx-eslint/eslint-plugin-react#3700](jsx-eslint/eslint-plugin-react#3700)

### ci

* add auto assign action ([#14](#14)) ([07d1f9a](07d1f9a))
* add check workflow ([#11](#11)) ([8afc82a](8afc82a))

### feat

* add [@eslint-community](https://github.com/eslint-community)/eslint-plugin-eslint-comments ([#17](#17)) ([fe2bf30](fe2bf30))
* add eslint-plugin-n ([#18](#18)) ([203d603](203d603))
* add eslint-plugin-security ([#16](#16)) ([e7f8c2e](e7f8c2e))
* add eslint-plugin-sonarjs ([#15](#15)) ([5bca4e1](5bca4e1))
* **react:** add some security linting ([#10](#10)) ([4424b67](4424b67))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants