joshwnj / react-visibility-sensor

Sensor component for React that notifies you when it goes in or out of the window viewport.
MIT License
2.32k stars 195 forks source link

Modernise codebase discussion #75

Open neeharv opened 7 years ago

neeharv commented 7 years ago

Looking to kickstart some discussion around modernising the code base and future features

Andarist commented 7 years ago

I could chime in with converting the codebase to es6 class which should be quite trivial to do. At the same time its no problem to rewrite some stuff to use more of es6. It would probably also mean some changes in the build process. I would suggest using just babel + rollup for umd builds. We'll look into the matter this weekend. Would only be cool to get some confirmation about this.

Im not sure if 'functional setState' is needed though. Its only (or are there other scenarios too?) useful when ur state depends on previous state. If it does not I think classical setState(obj) can be treated as a shorthand to the functional one.

IntersectionObserver support would be rly cool. Just wondering how much of a delay it causes as its an async beast and users need the asap calculations more often than not.

Also passive listener is quite a oneline change and probably should be incorporated asap. Or are there any downsides of them in this scenario?

neeharv commented 7 years ago

Im not sure if 'functional setState' is needed though. Its only (or are there other scenarios too?) useful when ur state depends on previous state. If it does not I think classical setState(obj) can be treated as a shorthand to the functional one.

The React team has repeatedly stated that the functional version of the setState API, while not perfect, is more in line with the direction of the future API. Moving to it is trivial and sets us up to add priorities for React Fiber's priority based async rendering.

IntersectionObserver support would be rly cool. Just wondering how much of a delay it causes as its an async beast and users need the asap calculations more often than not.

This is actually pretty interesting, because visibility checks are exactly why the Intersection Observer API was added. Instead of relying on user's running JS checks on every scroll event, or hacks like adding a check delay of x ms, asking the browser to do it for us should be the most performant way to do it.

Also passive listener is quite a oneline change and probably should be incorporated asap. Or are there any downsides of them in this scenario?

Unfortunately adding support for passive listeners is a little tricky as passing in an options param to the event listener causes issues in older browsers. There are ways to figure this out though, shouldn't be too hard. There isn't a downside AFAIK - here we aren't subscribing to scroll events to change the actual scroll behaviour (i.e , not calling preventDefault), so we don't need the overhead of pausing rendering.

meticoeus commented 7 years ago

You can check for passive scrolling support with this snippet:

var supportsPassive = false;
try {
  var opts = Object.defineProperty({}, 'passive', {
    get() {
      supportsPassive = true;
    },
  });
  window.addEventListener('test', null, opts);
} catch (e) { /* pass */ }

First came across it in react-chatview

neeharv commented 6 years ago

Have some time over christmas, so going to wrap this up! @joshwnj are you open to these suggestions?

helfer commented 6 years ago

Any progress here?