Open kamilmielnik opened 4 years ago
Yes, I see how having control over buckets would be useful, what I don't understand is
Furthermore, I think this parameter should be required (it should not default to the value of
ms
).
Why not make it default to ms
with ability to overwrite?
@streamich
Why not make it default to
ms
with ability to overwrite?
I find it surprising to have an assumption that all intervals with the same ms
param will be in the same bucket - I think it'd be better if the API required its' users to be aware and in control of how the grouping works to avoid surprises (i.e. bugs) as the code evolves.
I've recently encountered a bug which reminds me of this situation, and it could have been avoided if the "bucketing" was explicit from the start. Let me try to visualize it.
Imagine a component like this:
import React, { Component } from 'react';
const cache = {};
class CollectionCache extends Component {
constructor() {
state = cache;
}
componentDidUpdate() {
cache = state;
}
doSomething = (/* some params */) => {
this.setState(/* do something to state */);
};
render() {
const { children, ids } = this.props;
return children({
collection: this.state.map((id) => ids.includes(id)),
doSomething: this.doSomething,
});
}
}
It's all fine as long as CollectionCache
ids are globally unique. But if they're not, then there will be bugs if you want to use CollectionCache
for 2 different collections: entities from one collection would appear in the other.
A solution is to have a cache key (bucket id):
import React, { Component } from 'react';
const cache = {};
class CollectionCache extends Component {
constructor() {
state = cache[this.props.cacheKey] || {};
}
componentDidUpdate() {
cache[this.props.cacheKey] = state;
}
doSomething(/* some params */) {
this.setState(/* do something to state */);
}
render() {
const { children, ids } = this.props;
return children({
collection: this.state.map((id) => ids.includes(id)),
doSomething: this.doSomething,
});
}
}
Should the cacheKey
prop be required from the beginning, there would be no bug. But if the cacheKey
prop had a default value, the programmer could easily omit it and the bug would be there. Especially when using TypeScript, I find I'm lazy to check the documentation - if it compiles and seems to work, then I don't want to pass any more props.
I hope you can see the similarity of this problem. If not, I'll be happy to provide a working example of an issue.
All timers are grouped into "buckets" that are identified by the
ms
parameter. Using thems
parameter as a bucket identifier can have undesired consequences.My suggestion: add an extra
id
parameter and use it as bucket identifier instead ofms
parameter. Furthermore, I think this parameter should be required (it should not default to the value ofms
). Which would be a non-backwards-compatible change.Since, as I understand,
set-harmonic-interval
was developed to be used byreact-use
, let me usereact-use
as an example.Suppose you want to have 2 sets of components - numerous instances of
<X/>
and numerous instances of<Y/>
- and:<X/>
and<Y/>
to update after the same interval (samems
parameter value)X
to update when instances ofY
updateThis is impossible with the current implementation, because buckets are created per
ms
parameter.Hopefully the above explanation is sufficient but consider this example for completeness: https://codesandbox.io/s/tender-kepler-9r1j1?file=/src/App.js The stoplights will harmonically change the light every 5 seconds. The triggers will harmonically flash every 5 seconds as well. But triggers will not start flashing until you press any of them. And if it just so happens, that you push the trigger 100ms before the stoplights are about to change, the trigger indicators will be
on
only for that 100ms, while I'd expect them to beon
for the first 5 seconds after triggering.