add componentId - #349
Conversation
Closes bikeshaving#325 - Adds to components - Adds renderer options with to control id generation - adds tests to verify fast and stable mode produce valid identifiers The reason I think is valuable is that hashing the function.toString() can get expensive. It is also a warning in some security analysis tools. produces monotonic IDs, which will be internally stable but not stable across renders or instances (order could change which number a component gets). The default mode is , but I'm not married to this.
|
I just realized this won't discriminate between two different wrapped versions of a HOC. I'll add a test case, and try to come up with a solve for that in the morning. |
| // keys are roots | ||
| const afterMapByRoot = new WeakMap<object, Map<ContextState, Set<Function>>>(); | ||
|
|
||
| function djb2Hash(str: string): string { |
| generate: (fn: Function) => string; | ||
| } | ||
|
|
||
| const componentIdStates = new WeakMap<object, ComponentIdState>(); |
There was a problem hiding this comment.
This is an impressive usage of WeakMap that I’ll probably have to think about.
There was a problem hiding this comment.
You mentioned WeakMap in your original design, I figured this was exactly what you meant. Did you have something else in mind?
There was a problem hiding this comment.
I was spitballing. I enjoy a good WeakMap I guess.
brainkim
left a comment
There was a problem hiding this comment.
The only major concern I have is adding options to the Renderer interface. My instincts say pick a single, optimal component id system. Yes, figuring out the HOC situation is tricky, eager to see your solve.
|
Hey, sorry for the delay on updating this. I was one of the 40% of people laid off from Block yesterday. Yay 🙃 So, I added a test for componentIds from HOCs and... it works. I think I was letting late night brain do the thinking, because of course it works, at least in However, this seems like it should only work in // test harness
function Hoc(Component: any) {
return function Wrapped(props: any) {
console.log('HOC internal', this.componentId) // this is always the same
return <Component {...props} />;
};
}
const ComponentA = Hoc(function ComponentA(this: Context) {
idA = this.componentId;
return <div />;
});
const ComponentB = Hoc(function ComponentB(this: Context) {
idB = this.componentId;
return <div />;
});
const ComponentC = Hoc((props, ctx) => {
idC = ctx.componentId;
return <div />;
});So because There are other logs, for the string IDs from the HOC component, and they are all the same. This is not ideal, but the impact is not as bad as I thought it would be. Only the I imagine you want the function Hoc(Component: any) {
return function Wrapped(props: any) {
console.log('HOC internal', this.componentId) // this is always the same
return <Component {...props} />;
};
}One way to do that is to provide an API to the function Hoc(Component: any) {
function Wrapped(props: any) {
console.log('HOC internal', this.componentId) // this is always the same
return <Component {...props} />;
};
Wrapped.componentIdHash = Component.toString() + 'HOC';
return Wrapped;
}
// OR
function Hoc(Component: any) {
return function Wrapped(props: any) {
this.componentIdHash = Component.toString() + 'HOC';
console.log('HOC internal', this.componentId) // this is always the same
return <Component {...props} />;
};
}This isn't automatic, though it is still maybe the best solution. Here are some hacky ideas.
|
|
Darn about the current events! I’m sorry to hear that. Happy Friday: I will try to review this soon! I wish you success this year. I hope you show ’em what’s what. |
|
Happy Monday! This may be uninformed ignorance on my part, but what are the arguments against using referential identity for functions? That would be fast-ish (lookup in WeakMap on every componentId access) and stable? This is also going to likely be key for hot reload when we eventually open that can of worms. It’s very important to get right, so thank you for your thoughts. |
|
This does use referential identity for functions when looking up or storing the Stability in this context means that the hash will be the same when you
Getting those all to give you the same output means that the inputs cannot depend on the order components are rendered, since that could change on the server, or even on the client if async tasks resolve races differently. |
Closes #325
The reason I think
fastis valuable is that hashing the function.toString() can get expensive. It is also a warning in some security analysis tools.fastproduces monotonic IDs, which will be internally stable but not stable across renders or instances (order could change which number a component gets). The default mode isfast, but I'm not married to this.