Core: Don't recreate a bound story function each time we call a decorated story #14692
Conversation
tmeasday
requested review from
ndelangen,
shilman,
theinterned,
thomasbertet and
usulpro
as code owners
tmeasday
force-pushed
the
12255-fix-jsx-decorators-4
branch
from
e37fb51 to
3934b7c
Compare
|
@ndelangen I figured out some tests and it's passing 🎉 Here's the typing change: #14701 |
| export const defaultDecorateStory = ( | ||
| storyFn: LegacyStoryFn, | ||
| decorators: DecoratorFunction[] | ||
| ): LegacyStoryFn => { | ||
| // We use a trick to avoid recreating the bound story function inside `decorateStory`. | ||
| // Instead we pass it a context "getter", which is defined once (at "decoration time") | ||
| // The getter reads a variable which is scoped to this call of `decorateStory` | ||
| // (ie to this story), so there is no possibility of overlap. | ||
| // This will break if you call the same story twice interleaved | ||
| // (React might do it if you rendered the same story twice in the one ReactDom.render call, for instance) | ||
| let contextStore: StoryContext; | ||
| const decoratedWithContextStore = decorators.reduce( | ||
| (story, decorator) => decorateStory(story, decorator, () => contextStore), | ||
| storyFn | ||
| ); | ||
| return (context = defaultContext) => { | ||
| contextStore = context; | ||
| return decoratedWithContextStore(context); // Pass the context directly into the first decorator | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Do we need to make changes to the other decorateStory functions (vue has one, and I think angular has one too)
There was a problem hiding this comment.
Or is that simply not needed since the problem we're solving is react-specific?
There was a problem hiding this comment.
Not creating a new function is a general performance optimization though I think.. Should use less memory and fewer garbage collections needed.
Unless I'm wrong about that?
There was a problem hiding this comment.
I guess you are right. It does make the decorateStory function a little bit more complex and hard to reason about (perhaps that's debatable?) and the angular/vue/etc versions are sort of complicated already.
I'm open to changing it if you want, I'm not sure the perf difference is really going to be a big deal (stories don't get rendered all that often).
There was a problem hiding this comment.
I'd prefer making all decorateStory function be act similar, so that when you understand 1, you understand them all.
There was a problem hiding this comment.
Discussed this with @shilman.
We aren't super comfortable changing the already quite complex decorator "applicators" for other view layers. We don't understand the view layers well enough to know if it is a good idea or will change the behaviour in unexpected ways.
I think this will serve as a template for future view layers and if maintainers of other view layers want to bring it across as an optimization/simplification they can.
shilman
changed the title
Issue:#12255
What I did
Rather than using a closure and rebinding the story function with the context as it is decorated, instead use a getStoryContext() function which is defined once at decoration time to supply the context.
How to test
We have a Chromatic test. Will think about a Jest one
There is one
No