feat(popper): custom react wrapper

[bmdalex]

feat(popper): custom react wrapper

Problem description

Fixes #1342

We need to build a custom React wrapper around popper.js library (pure JS). The one offered by popper.js, called react-popper has performance issues: Performance issues on a large number of poppers #185:

React Bootstrap and Material UI are using popper.js as well and have decided to write their own React wrappers, https://material-ui.com/utils/popper/
(see: floating-ui/react-popper#185 (comment))

Proposed solution

Create a basic custom Popper component (similar to the one provided by react-popper) that:

• applies styles directly
• causes rerenders on small set of conditions (mostly props changes or calculated styles) but not on side effects coming from browser events
• reference: https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Popper/Popper.js

[codecov]

Codecov Report

Merging #1358 into master will increase coverage by 0.05%.
The diff coverage is 84.09%.

@@            Coverage Diff             @@
##           master    #1358      +/-   ##
==========================================
+ Coverage   72.93%   72.99%   +0.05%     
==========================================
  Files         775      776       +1     
  Lines        5823     5828       +5     
  Branches     1698     1706       +8     
==========================================
+ Hits         4247     4254       +7     
+ Misses       1570     1568       -2     
  Partials        6        6

Impacted Files	Coverage Δ
packages/react/src/index.ts	100% <ø> (ø)	⬆️
...hemes/teams/components/Popup/popupContentStyles.ts	23.07% <ø> (ø)	⬆️
...ages/react/src/lib/positioner/positioningHelper.ts	100% <100%> (ø)
...ckages/react/src/components/Popup/PopupContent.tsx	90.9% <100%> (ø)	⬆️
packages/react/src/components/Popup/Popup.tsx	67.74% <100%> (-1.17%)	⬇️
...ackages/react/src/lib/positioner/types.internal.ts	100% <100%> (ø)
packages/react/src/lib/positioner/Popper.tsx	76.66% <76.66%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07cb408...0350c2a. Read the comment docs.

[layershifter]

@Bugaa92 can you please link issue in react-popper repo with discussions of there problems?

[layershifter]

We should:

• remove react-popper from @stardust-ui/react dependencies
• add popper.js to @stardust-ui/react dependencies
• add react-popper to root package dependencies as we use it in prototypes
• ensure that we don't use any types from react-popper in lib/positioner

[bmdalex]

@layershifter

• remove react-popper from @stardust-ui/react dependencies

agreed

• add popper.js to @stardust-ui/react dependencies

agreed

• add react-popper to root package dependencies as we use it in prototypes

shouldn't we rather avoid using react-popper at all?

• ensure that we don't use any types from react-popper in lib/positioner

I checked, but how can we ensure that programmatically?

[layershifter]

shouldn't we rather avoid using react-popper at all?

We can, but we don't export Popper component now...

I checked, but how can we ensure that programmatically?

If we will remove the package 😼

[bmdalex]

shouldn't we rather avoid using react-popper at all?

We can, but we don't export Popper component now...

Should we try that? I'd do it, but not sure how. Do we want it?

I checked, but how can we ensure that programmatically?

If we will remove the package 😼

exactly my point :)

[kuzhelov]

no tests for this guy?

[bmdalex]

TBH I didn't really now HOW to test it. Reasons:

• PopperJS is setting styles directly on element so we have no control and no ability to predict anything there
• We could test returned placement but that's basically the result of getPlacement method that is already tested

Unless you want to test the options arguments we pass to PopperJS and mock the constructor? Open to suggestions.

[kuzhelov]

not sure why breaking change in the Popup's API was necessary? I may understand that in case if it would provide only beneficial effect in return - however, this solution seems to work nicely for Function Component + Hooks world. In case if it will be class component as consumer of Popup, it would require some tricky turns from the client to establish the variable to watch for - as, in contrast, callback-based approach would be the most intuitive thing.

I would suggest to leave this change aside from the PR, as it is not necessary to solve the PR's goal

[bmdalex]

not sure why breaking change in the Popup's API was necessary? I may understand that in case if it would provide only beneficial effect in return - however, this solution seems to work nicely for Function Component + Hooks world. In case if it will be class component as consumer of Popup, it would require some tricky turns from the client to establish the variable to watch for - as, in contrast, callback-based approach would be the most intuitive thing.

not sure why breaking change in the Popup's API was necessary?

It's not really necessary, but I find the existing renderContent API more confusing and this seemed like a good time to try another one and see how it goes.

I believe the new API is more discoverable by users and more clear in what it's trying to accomplish. positioningDependencies prop makes it obvious and explicit that this is the place where users can handle the positional updates.

this solution seems to work nicely for Function Component + Hooks world. In case if it will be class component as consumer of Popup, it would require some tricky turns from the client to establish the variable to watch for

Here's working code achieving the same scenario with function and class components. Doesn't really look like tricky stuff to me:

import * as React from 'react'
import { Button, Popup, Segment, Loader, Grid, Header } from '@stardust-ui/react'

const CustomSegment = () => (
  <Segment styles={{ minHeight: '300px' }}>Hello from loaded data!</Segment>
)

const AsyncDataLoader = props => {
  const [data, setData] = React.useState(<Loader />)

  React.useEffect(() => {
    setTimeout(() => {
      setData(<CustomSegment />)
      props.onLoaded()
    }, 1000)
  }, [])

  return data
}

const PopupExampleAsyncFn = () => {
  const [loading, setLoading] = React.useState(true)

  return (
    <Popup
      onOpenChange={(e, data) => {
        if (!data.open) setLoading(true)
      }}
      trigger={<Button icon="expand" content="Click me!" />}
      positioningDependencies={[loading]}
      content={{ content: <AsyncDataLoader onLoaded={() => setLoading(false)} /> }}
    />
  )
}

class PopupExampleAsyncClass extends React.Component {
  state = { loading: true }

  render() {
    return (
      <Popup
        onOpenChange={(e, data) => {
          if (!data.open) this.setState({ loading: true })
        }}
        trigger={<Button icon="expand" content="Click me!" />}
        positioningDependencies={[this.state.loading]}
        content={{
          content: <AsyncDataLoader onLoaded={() => this.setState({ loading: false })} />,
        }}
      />
    )
  }
}

const PopupExampleAsync = () => (
  <Grid columns={2}>
    <Header as="h3" content="PopupExampleAsyncFn" />
    <PopupExampleAsyncFn />
    <Header as="h3" content="PopupExampleAsyncClass" />
    <PopupExampleAsyncClass />
  </Grid>
)

export default PopupExampleAsync

in contrast, callback-based approach would be the most intuitive thing.

Hard to claim that without any user feedback. I checked some of our user's code and this API is not used at all..

Personally I had difficulties understanding how the renderContent approach worked and I don't like the fact that the users will need to switch from a prop to another (content -> renderContent). With current API it means just adding a prop to explicitly handle the fact that data is changed and we need to reposition content (positioningDependencies)

However, I don't feel too strongly about this and I am willing to hear other thoughts. I can agree to having this discussion on another PR. @layershifter , what do you think?

[DustyTheBot]

	Warnings
⚠️	Package (or peer) dependencies changed. Make sure you have approval before merging!

Changed dependencies in packages/react/package.json

package	before	after
popper.js	-	^1.14.6

Generated by 🚫 dangerJS

[kuzhelov]

nit: lets rename it to pointerTarget (and corresponding pointingTargetRefElement). Otherwise from the code it was unclear whether this implies element that is pointer itself (like arrow), or its target, or something else

[bmdalex]

but pointerRef is more aligned to current targetRef:

/**
 * Ref object containing the pointer node.
 */
pointerRef?: React.RefObject<Element>

/**
 * Ref object containing the target node (the element that we're using as reference for Popper box).
 */
targetRef?: React.RefObject<Element>

To me it seems that having pointerTarget and targetRef is more confusing

[kuzhelov]

PopperJS is improperly imported here - given that namespace is imported, it should be

import * as PopperJS from 'popper.js'

Currently the code transpiled to CommonJS will result in the following client's error:

[bmdalex]

I tried updating to:

import * as PopperJS from 'popper.js'

and ended up with this error:

If I change the constructor to new PopperJS.default I get a lint error that constructor cannot be lowercase and the code transpiled to CommonJS will result in the same error.

Are the types wrong? I'm blocked with this right now...
@layershifter @kuzhelov

[kuzhelov]

have checked popper.js module used in Stardust, as well as tried to repro the issue in Stardust environment - seems that everything is working fine, as well as types are properly defined for PopperJS module. Maybe this is a problem with client environment setup, where there is a stable repro for that - lets unblock this PR and see.

[kuzhelov]

@layershifter, what are your thoughts on that?