"Code review"

[Merri]

Hullo!

This isn't a real issue; instead I thought I'd drop a line since I used this project's code as a basis when implementing Intersection Observer into an older existing lib where I didn't want to have some of the baggage that this project currently has (or had), and I also wanted to do some minor changes to terminology that this project most likely doesn't want to include.

I think this project's implementation of Intersection Observer API for React is the best on npm, and I also think this project is now very close to version 1.0.0 as the Symbol issue was just removed. The weaknesses, from my point of view as an author who might want to use this project in their lib, are mostly in dependencies and use of modern code syntax that results into verbose results (as long as we need to keep babelizing and polyfilling).

I think it is ideal for any component that is used as a basis for other components to have as little dependency and as little unnecessary abstraction as possible. So, from this perspective here are some things that I changed when I refactored this project as part of react-lazy.

IntersectionObserverContainer.js

This goes a lot into opionated side of things, but I couldn't see any purpose for making IntersectionObserverContainer a class. Essentially all the methods are static functions and nothing is self-contained into the class itself. Thus I changed the file into intersectionObserver.js, removed class and renamed the functions to be a bit more verbose.

The advantage in this is reduced final code size. And there is also a little bit less confusion as well since IntersectionObserverContainer is not a React component, which one might first assume it to be.

The callback method

I had two thoughts related to the callback:

1. create method receives it, but ignores it's existance when calling getPooled method, yet it is required for new IntersectionObserver.
2. it feels a bit odd to find the callback method defined in IntersectionObserver component file since it relies more on internal mechanics of IntersectionObserverContainer file than what can be found in the component.

The first point introduces a possible future bug as callback is not checked against. Of course the current implementation always has the same callback, but the interface gives an impression that one could change it.

For the second point I think the callback method becomes easier to understand when it is moved to the other file as all the related Map and Set context is there.

Abstraction

Doing these changes would reduce code size.

• IntersectionObserverContainer.clear() and IntersectionObserverContainer.count() are only used for tests, simple to just remove as storage is exposed anyway
• compareObserverProps method in the component is easy to inline
• reobserve method in the component is used only once, easy to inline
• isDOMTypeElement is used only once, easy to inline

Unnecessary creation of new object in reduce

In component's get options is an unnecessary object spread: you can freely mutate an object you have created for a reducer.

warning is included for production build

I guess this should be inside a `process.env.NODE_ENV !== 'production' block. In the other hand onlyOnce is about to be removed so worth fixing only if it will take a while for the next major release.

Targetting

this.target, this.renderedTarget, this.targetChanged... I know these are very fast to access if compared to calling this.setState, but I can't really avoid the thought that the code could be simpler with setState.

PureComponent?

Current target management prevents using this. With setState this would become a viable solution to the shallow props checking. Of course it isn't exactly the same: the shouldComponentUpdate implementation doesn't check the contents of an array, only the reference. Maybe threshold could be a string instead of an array to make things easier?

Final thoughts

For reference, here are links to how I refactored those files:

• IntersectionObserverContainer.js -> intersectionObserver.js
• IntersectionObserver.jsx -> Observer.jsx

I did other minor changes as well which might be interesting to see, but there are so many changes (and I don't use semicolons) that diff isn't much of help. Most of the code should seem very familiar as it does implement the exact same logic, but only does it with either slightly faster means or code that results in reduced babelized code size or less dependencies.

And then there is also the addition of viewport (root) and cushion (rootMargin) props, which seem like easier to understand what those props do than what IntersectionObserver options say.

Anyway, hopefully these thoughts are of some value :)

[Rendez]

Hey @Merri!

This is one of the most valuable issues I’ve received on Github thus far. Thank you for doing such a thorough code review, it's great!

There're very good points, so let me say first off that I agree with the majority of them. I'd like to include these changes – probably with the exception of terminology for obvious reasons – but I want to start with leveraging code size reduction and better organization and inlining of code.

Ideally we’d implement enough of your suggestions, so that react-lazy can include this package as one of its dependencies. If you think this is possible, I’d like for either you or us to open a PR soon, so that we can release a new version containing all the changes.

So let’s break down your suggestion into actionable points – let me know later if I left anything out:

IntersectionObserverContainer.js

I measured the uglified Babel output (after removing the runtime assuming it’s in a shared module) and yours comes out almost 200 bytes lighter 👍

Observer callback

Do not see the part of the future bug, but it should be moved to the above-mentioned file indeed 👍

Abstraction

• clear/count can be removed 👍
• compareObserverProps is idiomatic, but it can be inlined if you want as a variable with the same name 👍
• reobserve can be inlined 👍
• isDOMTypeElement is used once, but as a separate method makes is easier for testing, I would leave it (I don’t feel too strong about leaving it either, but it’s nice to ensure the child is valid in such a clear way).

New object in reduce

Yep, I didn’t see that one 👍

Warning

I also thought it’d be removed soon, but later I decided it might stay a while, since probably too many people have relied on onlyOnce on their codebases at this point. So let’s IF for NODE_ENV too 👍

Targetting

The problem with setState is that we’d be calling render() again. Despite the impression that we’re 100% declarative, we're still working with an imperative API and side-effects coming from the DOM. IMO setState must reflect the state of the UI, not an internal logic flag. I think we can be efficient at comparing whether the DOM node has changed, only by saving the previous+next targets. If we compare children, React will never compare them deeply, so you will have different children elements – unless you store your element object reference in another place and you keep it in memory for further re-renders.

PureComponent

Unfortunately, PureComponent can’t compare children efficiently like I was saying. It's not the react-way to optimise things, to store a reference either. We won’t be able to determine if we need to re-observe a DOM node unless we re-render, because that node reference is inside the ref callback, so that's what we do.

Instead, when we use <Observer> inside a parent component, this parent usually takes charge of the bailing out part. At RG for instance, these parents are quite often HOC-wrapped – with connect() from redux or similar.

You can apply PureComponent on your parent components the way you prefer it, provided you know that the child node you’re observing will never change, and this is the case almost every time.

I’m extending myself quite long here, but the point is that you probably shouldn’t be re-using the observer for different target types, and if you do, we expect the correct behavior to happen.

Semicolons

I don’t care. I really don’t care, let’s remove them, let’s add them… Post-processing will get rid off most of them anyways.

Terminology

I am pretty sure you can keep viewport and cushion on your components <Lazy> and <LazyGroup> without changing it here; you don't even use <Observer> directly on your demo. Our terminology is the Native API one, and I'd like to keep it that way so that no one has to remember two APIs.

[Merri]

Great :)

I guess one way to go would be to create a pull request with a lot of micro size commits, then let you decide which ones of the changes you want to take and leave out the commits that don't make the cut (since there are some less important changes/suggestions). Then once there is a new version out I'll update react-lazy and add this package to the peerDependencies. Sounds like a plan? Or would you rather want pull requests that each have a specific theme?

Callback

The bug with callback is that if it was possible to actually change the callback then it would be become possible that an incorrect instance of IntersectionObserver would be given for options that match existing IntersectionObserver, but have a different callback, since there is no check against it. This is why I think callback shouldn't be passed to the create method as the code assumes it to always be the same.

isDOMTypeElement

To me testing this is testing a React feature and it shouldn't be a test concern for this component. This method would only break if React changed a lot, in which case they are probably responsible enough to create a new method instead of breaking an old one.

Target

This one (and PureComponent) were the things I were least sure of suggesting as I didn't go ahead and write any code. There is something that bothers me with using three variables for what essentially should need only two variables (past and present). Also, the fact there is mutative code in render method makes me think there would be something wrong.

If I understand the cycle correctly off the top of my head, it goes render -> handleNode -> componentDidUpdate. If this holds true then this.unobserve() is called two times when target changes. This then makes me think that handleNode should only be concerned about setting this.target and componentDidUpdate should be the one that checks whether the target has changed and then track the previous target. Furthermore this would remove the need for this.targetChanged and also remove this.renderedTarget from render. I'd also rename it prevTarget as that would match with React convention.

But this is again where I assume a lot without testing whether my understanding is correct :)

[Rendez]

I guess one way to go would be to create a pull request with a lot of micro size commits

Sounds like a plan. Looking forward to it 🎉

If I understand the cycle correctly off the top of my head, it goes render -> handleNode -> componentDidUpdate. If this holds true then this.unobserve() is called two times when target changes

That's right, we need to stop observing it before the node is removed from the DOM. This part was thought out way before methods like getSnapshotBeforeUpdate() were introduced, and I decided to live with the fact that it's called twice (at a very low runtime cost).
If we do this, I have to introduce react-lifecycles-compat, unless you have a suggestion that's compatible with React 15.

I think it might be enough to just modify cDU like so:

componentDidUpdate(prevProps) {
    if (this.targetChanged) {
        if (!this.props.disabled) {
            this.observe();
        }
    } else if (this.compareObserverProps(prevProps)) {
        this.unobserve();
        this.observer();
    }
}

Thoughts?

[Merri]

Hmmh, I guess that forces the boolean variable. Doesn't seem worthwhile to change the code to be "proper" and make react-lifecycles-compat a hard requirement as the current solution does work. With this about the only suggestion would be to document this detail in the code with comments and maybe make some kind of plan when to drop support of pre-16.3 versions of React so that it can then be fixed :) Seems quite hard to find React version share statistics.

I try to fork and start throwing commits together during the weekend. Had to wake up earlier than usual which isn't ideal for getting anything done.