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

[ButtonBase] Fix when changing enableRipple prop from false to true #19667

Merged
merged 13 commits into from
Feb 26, 2020
Merged

[ButtonBase] Fix when changing enableRipple prop from false to true #19667

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f546941
Fix crashing if rippleRef is null
dmtrKovalenko Feb 12, 2020
c9e7761
Add test that reproduces crash
dmtrKovalenko Feb 24, 2020
285a7df
Fix crashing by moving <NoSsr /> outside of rendering conditino
dmtrKovalenko Feb 24, 2020
d17f1a1
Get rid of autoFocus button in tests
dmtrKovalenko Feb 24, 2020
fefc0af
Ignore typescript errors in test
dmtrKovalenko Feb 24, 2020
95a3237
Run prettier in ButtonBase.test.js
dmtrKovalenko Feb 24, 2020
07a312f
Get rid of @ts-ignore
dmtrKovalenko Feb 24, 2020
85a4ae5
Fix syntax error in typescript type comment alias
dmtrKovalenko Feb 24, 2020
f646dbb
Throw error if buttonRef is missing in test
dmtrKovalenko Feb 25, 2020
9ae49af
Allow chilren={null} in <NoSsr />
dmtrKovalenko Feb 25, 2020
d63a6ad
yarn prettier
oliviertassinari Feb 26, 2020
9f2921b
yarn docs:api
oliviertassinari Feb 26, 2020
1f124be
let's not forget about the TypeScript types
oliviertassinari Feb 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/material-ui/src/ButtonBase/ButtonBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,12 +302,12 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
{...other}
>
{children}
{!disableRipple && !disabled ? (
<NoSsr>
{/* TouchRipple is only needed client-side, x2 boost on the server. */}
<NoSsr>
{!disableRipple && !disabled && (
/* TouchRipple is only needed client-side, x2 boost on the server. */
<TouchRipple ref={rippleRef} center={centerRipple} {...TouchRippleProps} />
</NoSsr>
) : null}
)}
</NoSsr>
</ComponentProp>
);
});
Expand Down
46 changes: 46 additions & 0 deletions packages/material-ui/src/ButtonBase/ButtonBase.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,52 @@ describe('<ButtonBase />', () => {
button.querySelectorAll('.ripple-visible .child:not(.child-leaving)'),
).to.have.lengthOf(0);
});

it('should not crash when changes enableRipple from false to true', () => {
function App() {
/** @type {React.MutableRefObject<import('./ButtonBase').ButtonBaseActions | null)> | null} */
const buttonRef = React.useRef(null);
const [enableRipple, setRipple] = React.useState(false);

React.useEffect(() => {
if (buttonRef.current) {
Copy link
Member

Choose a reason for hiding this comment

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

This is the type of defensive checks I was working about that TypeScript encourage and that might hide root issues. @eps1lon what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is not hiding issue of test, you can check it and without changes this test will still crash

Copy link
Member

Choose a reason for hiding this comment

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

Agree, I was wondering about the pattern in general, what should be our "baseline" (default approach) for this concern :)

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need this check here? What happens if App mounts but the ref isn't instantiated?

Copy link
Member Author

@dmtrKovalenko dmtrKovalenko Feb 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This check is for typescript compiler, to get rid of @ts-ignore. Actually it is impossible inside the useEffect (maybe only if somebody will specifically delete dom element after react commit phase and before effects are run?)

Right here it is not needed, but I am sure that in the code we have to guard the refs, even if it is hide bugs (IMHO such guards can't hide bugs, because e.g. in this particular issue element will not gain ripple focus if check was done - it is a bug as well, but no so critical as crash)

Copy link
Member

Choose a reason for hiding this comment

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

it is a bug as well, but no so critical as crash)

So we agree that it shouldn't be guarded against. There was a defect in the code which we wouldn't have been able to detect with a defensive check.

As long as typescript is only a type checker + transpiler I don't care about @ts-ignore in JSX. TypeScript is particularly bad with React. If it complains, we disable it. Just like with false positives in lint rules.

Copy link
Member

Choose a reason for hiding this comment

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

We still need to get rid of this check in the test or handle the else case.

Copy link
Member Author

@dmtrKovalenko dmtrKovalenko Feb 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

👌 ok I will add throwing error in the else branch.
Cannot agree — we would be able. If there will be a guard, when enableRipple changes we simply not starting rippleEffect. It is an issue as well that can be noticed and fixed in the same way as current one.

But if some user will notice crash in the most basic Button component — it will spoil the trust to material-ui as qualified and reliable tool.

I have strong opinion that if something can crash — you must not let it crash. Even if you will use more reliable type system like ReasonML — it will complain about refs, because we cannot trust DOM, like we cannot trust users and we cannot trust developers who uses our components.

I can propose you the following api for using in the core components.

assertReactDomRef(ref, ref => ref.focus())
// or using new asserts syntax
assertReactDomRef(ref)
ref.focus()

Which will assert ref is not null and if it’s missing warn user and ask to raise an issue. Thus our component will even look more clever :)

Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand the issue, this is an internal consideration. Regarding the impact userland, I think that a fast feedback loop should be preferred.

Copy link
Member

@oliviertassinari oliviertassinari Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@dmtrKovalenko I love the new throw error in the branch, it's a great edge in case the action prop stop working as expected, we get a clear error message in the test. Smart! In an older test, we were using // @ts-ignore, which sounds great too.

But if some user will notice crash in the most basic Button component — it will spoil the trust to material-ui as qualified and reliable tool.

From my perspective, the ref effect should always run before a layout effect which also runs before an effect. The ref should always be defined. If it's not, then there is a deeper issue, that a defensive logic will hide, make it harder to uncover.

So in userland, I would recommend the usage of // @ts-ignore or to throw, like in the test cases.

buttonRef.current.focusVisible();
}
}, []);

return (
<div>
<button
type="button"
data-testid="trigger"
onClick={() => {
setRipple(true);
}}
>
Trigger crash
</button>
<ButtonBase
autoFocus
action={buttonRef}
TouchRippleProps={{
classes: {
ripplePulsate: 'ripple-pulsate',
},
}}
focusRipple
disableRipple={!enableRipple}
>
the button
</ButtonBase>
</div>
);
}

const { container, getByTestId } = render(<App />);

fireEvent.click(getByTestId('trigger'));
expect(container.querySelectorAll('.ripple-pulsate')).to.have.lengthOf(1);
});
});
});

Expand Down