Prepare gestalt for eslint 5 via bump to plugins #273
Conversation
|
Deploy preview for gestalt ready! Built with commit 97ebed4 |
|
Deploy preview for gestalt ready! Built with commit 0bdccd7 |
Generated by 🚫 dangerJS |
| @@ -54,6 +54,7 @@ export default function Button(props: Props) { | |||
| [styles.block]: !inline, | |||
| }); | |||
|
|
|||
| /* eslint-disable react/button-has-type */ | |||
There was a problem hiding this comment.
@pfarejowicz Any idea why we need to disable this even though we set it below? Is it because type is dynamic?
type={type}
There was a problem hiding this comment.
@christianvuerings exactly! the rule does not like us using the dynamic value type={type} even if it is Flow typed and prop typed to match the possible values. There's more discussion in jsx-eslint/eslint-plugin-react#1555 and the recommendation is to fork the component and render two different JSX components but to me that feels like overkill, what are your thoughts? For reference:
I'd say that the rule should be forcing a single, hardcoded type value - and that you should conditionally render two different elements if you want two types.
There was a problem hiding this comment.
Disabling it here seems fine - thanks for clarifying
packages/gestalt/src/Flyout.test.js
Outdated
| @@ -6,7 +6,11 @@ import Flyout from './Flyout.js'; | |||
| it('Flyout renders', () => { | |||
| const wrapper = shallow( | |||
| <Flyout | |||
| anchor={<button onClick={() => null}> test </button>} | |||
| anchor={ | |||
| <button onClick={() => null} type="submit"> | |||
There was a problem hiding this comment.
@pfarejowicz we should probably change this to type="button". A flyout inside of a form should not be submitting the form. Also button is the default for <Button>
| @@ -36,6 +36,7 @@ export default function SegmentedControl(props: Props) { | |||
| key={i} | |||
| onClick={e => onChange({ event: e, activeIndex: i })} | |||
| role="tab" | |||
| type="submit" | |||
There was a problem hiding this comment.
@pfarejowicz could we change this to type="button"?
There was a problem hiding this comment.
I can make the change, I agree button is better than submit. My goal was to introduce the minimal amount of changes and since this is a <button /> and not a <Button /> the default here was actually always setting this to type="submit" since we were not specifying it explicitly. See the Mozilla docs for reference. Making this change now, but just making a not here for future readers that the behavior is changing from our released version.
type
The type of the button. Possible values are:submit: The button submits the form data to the server. This is the default if the attribute is not specified, or if the attribute is dynamically changed to an empty or invalid value.
test/containers/ExampleGridItem.js
Outdated
| @@ -48,6 +48,7 @@ export default class Item extends Component { | |||
| <button | |||
| id={`add-more-${itemIdx}`} | |||
| onClick={() => fetchMore().then(addRelatedItems)} | |||
| type="submit" | |||
There was a problem hiding this comment.
@pfarejowicz same here - change to type="button"
There was a problem hiding this comment.
Done, same comment as above but this is a test component so Im not too concerned
Codecov Report
@@ Coverage Diff @@
## master #273 +/- ##
=======================================
Coverage 70.24% 70.24%
=======================================
Files 46 46
Lines 820 820
Branches 189 189
=======================================
Hits 576 576
Misses 172 172
Partials 72 72
Continue to review full report at Codecov.
|
| @@ -56,7 +56,8 @@ export default function transformer(file, api) { | |||
| if (convertSizes[value]) { | |||
| attribute.value.value = convertSizes[value]; | |||
| return attribute; | |||
| } else if (invalidSizes.includes(value)) { | |||
| } | |||
| if (invalidSizes.includes(value)) { | |||
There was a problem hiding this comment.
@chrislloyd same reaction haha, its this rule https://eslint.org/docs/rules/no-else-return which does not like an else if clause after a return
packages/gestalt/src/Button.js
Outdated
| } | ||
|
|
||
| Button.defaultProps = { |
There was a problem hiding this comment.
I'm so torn on this one. None of our tools really introspect defaultProps and it's not a required part of the React API so this definitely feels like extra typing...
There was a problem hiding this comment.
Agreed - I started off by playing with the typing and defaulting, hoping to fix the type={type} error but to no avail since I found the github thread linked above saying it needs to be hardcoded to "button" or "submit". Figured I would keep the defaultProps that I created anyways since it is a common pattern. Thoughts? I can revert the defaulting since the rule needs to be disabled anyways
ESLINT 5 is out, but we cannot upgrade because the eslint-plugin-airbnb latest version has a peer dependency on eslint 4.19.1 still. Trying to upgrade straight to eslint 5 leads to lots of peer dep errors from yarn. The fix has already landed on the airbnb master but it has not been released yet.
Tracking airbnb/javascript#1834 for when we can finally update to eslint 5!
In the meantime, this change:
react/destructuring-assignmentrule temporarily as that one is a big offender ~80 times so I will handle it on its ownbutton-has-typerule onButtonbecause apparently the creators want developers to hard code values and duplicate code instead of using the dynamic type. See button-has-type does not allow for dynamic assignment jsx-eslint/eslint-plugin-react#1555 for more infobuttontypeI used thesubmitvalue as that is what the default behavior is if the attribute is ignored according to HTML docs