Code Review

[Merri]

Hello! This is a neat addition for use with Styled Components!

I have a tendency to look at code before using stuff and noticed surprisingly many dependencies, and after having a more in-depth look I also noticed some notable performance issues with the current implementation, so I thought to bring them up.

Size: domElements.js can be removed

You can use Object.keys(styled) to get the list of DOM elements.

Perf: ResizeObserver should be re-used for all instances

This has been noticed to be better for performance than making a new instance each time. I've read so many container query texts in the past few days that I can't remember which one(s) pointed this out :)

Perf: unitToPx should have px as the first case

This is a minor optimization, but since 99% of use-cases are likely to be px it makes a lot of sense to check for it first.

Perf: this.parseQueryUnits should only be called when this.props.query changes

Now it is being called upon each resize event.

Size & Perf: this.setState is called even if nothing has changed

Now each pixel change will trigger render, because array is always a new reference. A way to fix this is to change additionalClassNames into a string, after which it is very simple to check if state has changed before calling setState.

You can also drop being dependent on classnames because you can simply store additionalClassNames as either empty string or as a string with space prefix already added before the active classes.

Size & Perf: Avoid React.cloneElement to remove re-processing of an element

withQueryContainer is already doing render of <Component /> and this is the better place to set it's className. This means state has to be moved up from QueryContainer to reside within withQueryContainer. This will also reduce code size as you will avoid the complex React.Children mapping.

Perf: You can pre-calculate entries for matchQueries

This is unnecessary work to be done each time matchQueries is called: it can be done a single time when this.props.query changes.

You can also optimize matchQueries so that instead of an array it returns a string: simply use .reducer instead of .filter and .map. You don't even need a special checking for empty string as you can prefix each active className with a space (if implementing the suggestion to remove classnames dependency).

I would also give width and height as direct arguments instead of destructuring.

Size: Removing resize-observer dependency

While including this makes using this library easier, it is also a forced large polyfill to be included, and user may already have it, or may serve the polyfill conditionally using a service like polyfill.io. So it would be better to remove this and instead instruct users to serve a polyfill if they need to support browsers which don't have ResizeObserver.

General wondering

In current design a styled component is created first and then passed to withQueryContainer HOC. What if things were in reverse and Component was wrapped for observing before passing it to styled components? Would this remove the need to use hoistStatics?

I hope this isn't too much stuff for one issue :)

[Merri]

Ended up making a refactor, here is a CodeSandbox. It does changes on most of what was mentioned above and uses React hooks instead of a class.

For the moment I have no plans to release this. It would be very nice to solve passing props callbacks and using objects in addition to templates :)

[FreddyFY]

Hi @Merri

Wow. Thanks for all your inputs. I will check them out in the next days.