Remove componentDidMount and componentDidUpdate hooks from <Popover>

[ab-pm]

While using <Popover> in controlled mode to work around #6171, I noticed that it always renders twice when onInteraction changes my isOpen state. On first render, renderTarget is called with outdated arguments (closed instead of open), only immediately after in the second render it gets passed the correct values.
You can repro with something like

<Popover
	…
	isOpen={isOpen}
	onInteraction={(nextOpenState, event) => {
		console.log('interaction', nextOpenState, event);
		setIsOpen(nextOpenState);
	}}
	renderTarget={props => {
		console.log('Rendering popover target', isOpen, props);
		return …;
	}}
/>

I traced the problem down to setState being called from componentDidUpdate. Instead the component should simply take the isOpen value from the props and ignore its local state altogether in controlled mode.

Checklist

• Includes tests
• Update documentation

Changes proposed in this pull request:

• avoid calling setState from componentDidUpdate, which leads to the component rendering twice in controlled mode
• instead, just ignore local state when the component is in controlled mode
• remove componentDidMount and componentDidUpdate hooks from <Popover> altogether
• assert that onInteraction is always called with an event
• also remove hasDarkParent and isClosingViaEscapeKeypress from react component state, put them in class properties instead and update them whenever the overlay is opening/closing

Reviewers should focus on:

• dark mode and esc key behavior
• coding style (I'm unfamiliar with Bluprint guidelines)
• testing (I've only done this in the web editor, haven't actually installed the dev environment and ran any tooling)
• whether no longer calling onInteraction due to props change is a bugfix or a compatibility issue

[palantirtech]

Thanks for your interest in palantir/blueprint, @ab-pm! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[changelog-app]

Generate changelog in packages/core/changelog/@unreleased

What do the change types mean?

• feature: A new feature of the service.
• improvement: An incremental improvement in the functionality or operation of the service.
• fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
• break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
  and external consumers of the service's API (e.g. customer-written software or integrations).
• deprecation: Advertises the intention to remove service functionality without any change to the
  operation of the service itself.
• manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
  performing database surgery, ...) at the time of upgrade for it to succeed.
• migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

• ❗The break and manual task changelog types will result in a major release!
• 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
• ✨ All others will result in a minor version release.

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Remove componentDidMount and componentDidUpdate hooks from <Popover>

Check the box to generate changelog(s)

• Generate changelog entry

[ab-pm]

@palantir do you need anything else? Do I need to provide a changelog entry before the PR even is reviewed?

[ericjeney]

Nothing else necessary, and you can ignore the changelog check! We should be able to give this a review next week.

[gluxon]

Thank you! This fix is very appreciated. I was also looking at #6771 recently, which was another case where we should have simply done a simple nullish coalesce like you did instead of complicated state handling.

blueprint/packages/core/src/components/popover/popover.tsx

Line 246 in 22355c2

return this.props.isOpen ?? this.state.isOpen;

Pure Rendering

I'm a bit nervous about the other changes in this PR that move the hasDarkParent and isClosingViaEscapeKeypress state booleans to be instance variables. I'm not sure if that change will work as expected with React 18's concurrent renderer, which expects the render output of a component to be a pure function of its state and props.

https://react.dev/reference/react/StrictMode#fixing-bugs-found-by-double-rendering-in-development

React assumes that every component you write is a pure function. This means that React components you write must always return the same JSX given the same inputs (props, state, and context).

I acknowledge that you'll probably find existing Blueprint code that violates this best practice since it only became a stronger recommendation as of React 18. We shouldn't actively move away from this best practice in new code though.

Request

Could we revert the changes to hasDarkParent and isClosingViaEscapeKeypress for now? If there's factors I'm missing that make this safe to do, I'd be open to hear those explanations. Thanks.

[ab-pm]

React 18's concurrent renderer expects the render output of a component to be a pure function of its state and props.

I'm under the impression that guidance is a bit simplified and pertains only to function components. For class components, it would need to talk about the render method, which gets the whole instance as its input this argument and is still pure in terms of that. Rendering a Popover does neither mutate the instance (or its .state or .props) and does not have any side effects either. Notice the instance properties are only updated from event handlers. So I'm pretty sure the code is still on the safe side here.

Admittedly, the guidance does not talk about rendering of useRef values, which would be the function component equivalent to class component instance properties. It's probably not the best idea since changing a ref/instance property does not cause the component to re-render, but I think it works fine here. targetHasDarkParent and isClosingViaEscapeKeypress would only ever change when the popover opens or closes, not on renders in between, right?

Tbh I'm more concerned whether moving the targetHasDarkParent update from componentDidMount/componentDidUpdate to the open/close handler works properly (regardless whether it's a state or property update). I'm not sure how this is meant to work, and have no resources to test this on my own. In particular, I might have broken it for controlled popovers? Tbh the whole approach seems sketchy to me - https://legacy.reactjs.org/docs/strict-mode.html for example did mention that side effects (like state updates) from componentDidMount/componentDidUpdate should be avoided, but I don't have a better solution either. In a function component I'd probably have used a layout effect, or even better just relied on a context for dark mode, but I didn't want to do an entire refactoring in this PR :-)