styled-container-query
styled-container-query copied to clipboard
Code Review
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 :)