Core: Add option to use webpack filesystem cache #16219
Conversation
Nx Cloud ReportCI ran the following commands for commit 2d08b3f. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch
Sent with 💌 from NxCloud. |
|
Perhaps we need an "optimization" section of the docs or something, WDYT @jonniebigodes? |
There was a problem hiding this comment.
@tmeasday i agree, this is looking great.
i could also imagine an alternative that's consistent with our presets API:
export type BuilderConfig =
| string
| {
name: string;
options?: unknown;
};
interface CoreConfig {
builder?: BuilderConfig;
}WDYT?
|
Sure thing! |
|
@tmeasday yes, indeed we'll need to add something to the docs. @literalpie once this is merged and available, are you ok with following up with me on updating the docs and mentioning this feature? Let me know and we'll adjust accordingly! Have a great day! Stay safe |
|
@jonniebigodes sure, I can do that |
|
@shilman @tmeasday are we still hoping to get this in 6.4? It sounds like an RC is coming soon, but there hasn't been movement on this in a while. I hope this hasn't been held up waiting for me to do something. My understanding is that I'm only expected to add documentation after this gets merged. Is that right? Or should I update the PR to match shilman's comment? |
|
@literalpie If you can take a stab at my comment, I can't promise we can get it into 6.4 but that's the next step here. @tmeasday probably also has thoughts on this |
literalpie
force-pushed
the
fsCache
branch
3 times, most recently
from
a14f043
to
48c1b2d
Compare
|
@shilman I believe I've updated this to do what you asked. I think I even improved the types some beyond what you suggested (but I can match your suggestion exactly if you want). I tested it in web-component-kitchen-sink again with the new config format, and it works as expected. Also, if we decide not to include this in 6.4 for any reason, I could always just publish an addon that adds the webpack config. I've been using the config in my projects for a while, and the only issues I've had are warnings that certain things are not serializable (for example, sass errors in an angular project) |
| if ((coreConfig.builder as Webpack5BuilderConfig).options?.fsCache) { | ||
| logger.line(1); // force starting new line | ||
| logger.info('=> Ignoring Storybook cache because webpack cache is being used'); | ||
| } |
There was a problem hiding this comment.
Hmmm, I'm not sure about this. The manager cache is probably a bigger speedup than the WP cache... 🤔
There was a problem hiding this comment.
Yes, in my informal testing, i think the storybook manager cache was about 2 seconds faster than webpack. I thought we might still want to prefer webpack since it's more official, and less code for storybook to maintain.
Let me know if you definitely want to remove it.
There was a problem hiding this comment.
I'm really not sure! Sorry if I was a bit terse, I wasn't necessarily disagreeing with this, just pondering. WDYT @ghengeveld / @shilman?
I agree simplicity is better, I would definitely say we should only have one cache on the manager side, either WP's or our own.
|
Updated with changes requested by @tmeasday . I didn't remove the skipping of the SB manager cache, since I think that's still an open question. |
|
Thanks @literalpie! Will sort it out ASAP. |
|
OK @literalpie discussed with @shilman and we think it'll be easier and more predictable for us if we just don't use the WP cache in the manager. That way all code paths are the same no matter which WP builder they use with which options. |
|
Sounds good! Thanks for letting me help! |
shilman
changed the title
Issue: #16072
What I did
I have this feature working, but I am not confident that I have done it in the best way. I am open to feedback, or allowing someone else to take over this feature.
How to test
node_modules/.cache/webpackis created and includes cache files.