storybook
storybook copied to clipboard
Build: Replace globby with fdir
Issue:
What I did
I was trying to upgrade our version of jest from 26 to 29, and I started getting failures of:
setImmediate is not defined
These were coming from globby
(or one of its dependencies).
I could have solved this in three ways, I think:
- install a polyfill of setImmediate. Seemed like the wrong approach.
- configure those failing jest tests to run with a node environment rather than jsdom. We may still want to explore this, but I need to figure out how to configure some of our tests to run with node environment and others with jsdom, since we have packages that expect to run in one or the other or both environments.
- replace globby with something else that doesn't have this problem.
I did some checking around, and while tiny-glob
seemed like a promising candidate, it didn't have a way to ignore node_modules (https://github.com/terkelg/tiny-glob/issues/79).
I then came across fdir
, which claims to be the fastest glob solution around, with no dependencies and a small size. https://github.com/thecodrr/fdir
This PR swaps out globby for fdir. I think every thing we can do to gain some perf is a win, but happy to explore the other alternatives here as well.
How to test
- [ ] Is this testable with Jest or Chromatic screenshots?
- [ ] Does this need a new example in the kitchen sink apps?
- [ ] Does this need an update to the documentation?
If your answer is yes to any of these, please make sure to include it in your PR.
I ran fdir's benchmarks, which include fast-glob (globby) and tiny-glob, and it came out on top:
Running "Synchronous (1487 files, 230 folders)" suite...
Progress: 100%
fdir 5.2.0 sync:
90 ops/s, ±0.33% | fastest
glob sync:
16 ops/s, ±0.62% | slowest, 82.22% slower
fast-glob sync:
71 ops/s, ±0.67% | 21.11% slower
tiny-glob sync:
45 ops/s, ±0.20% | 50% slower
Finished 4 cases!
Fastest: fdir 5.2.0 sync
Slowest: glob sync
Any idea what this pnp e2e failure could be from?
Error: Qualified path resolution failed: we looked for the following paths, but none could be accessed.
ERR!
ERR! Source path: /tmp/storybook-e2e-testing/cra/.yarn/__virtual__/@storybook-addon-interactions-virtual-b7e2a61c70/4/root/.yarn/berry/cache/@storybook-addon-interactions-npm-7.0.0-alpha.34-d8e351c364-8.zip/node_modules/@storybook/addon-interactions/dist/cjs/preset/checkActionsLoaded
I've broken out a separate PR to fix the issues with addon-interactions
: https://github.com/storybookjs/storybook/pull/19334
Waiting for https://github.com/thecodrr/fdir/pull/80 to be released, which should fix yarn pnp.
I'll try making these calls async as well.
Socket Security Report
👍 No new dependency issues detected in pull request
Socket.dev scan summary
Issue | Status |
---|---|
Did you mean? | ✅ no new possible package typos |
Install scripts | ✅ no new install scripts |
Telemetry | ✅ no new telemetry |
Troll package | ✅ no new troll packages |
Malware | ✅ no new malware |
Native code | ✅ no new native modules |
Bot Commands
To ignore an alert, reply with a comment starting with @SocketSecurity ignore
followed by a space separated list of package-name@version
specifiers. e.g. @SocketSecurity ignore [email protected] [email protected]
⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.
Powered by socket.dev
Merge when green!
Unfortunately I've hit another snag with fdir. The way it handles symlinks is by resolved path, not the symlink path. This breaks in our testing setup, where we symlink in various addon stories.
The author is considering changing this behavior, but until/unless he does, I think we can't move forward with fdir. I may need to put this back down for a while and focus on other parts of the jest upgrade, and come back to this later either when fdir's symlink behavior is changed, or find a different alternative, or maybe just find a way to adjust jest to work correctly with globby, our existing tool.
I found a different way to handle the underlying issue with setImmediate. We may want to re-evaluate the use of globby in the future, but given the challenges I've had getting fdir to work in an equivalent way, I'm closing this out for now.
@IanVS
Would you be up for taking another look at this? There have been a lot of changes in the way fdir
works. Especially everything around globbing
is now much improved and works as expected.
A few examples:
- Everything was rewritten in TypeScript
-
fdir
now doesn't resolve symlinks by default (but requireswithSymlinks
option) -
globbing
now supports changingpicomatch
options with the.globWithOptions
function. This fixes the behavior you mentioned in https://github.com/thecodrr/fdir/pull/85 - A lot of other issues with globs were fixed.
The reason I am bringing this up again is due to the performance benefits Storybook can get by migrating to fdir
. In your own benchmarks above, fdir quite a bit faster than the competition. If you like I can take a stab at renewing this PR.
Thanks for your work on fdir, @thecodrr. I think this PR is probably quite a bit outdated at this point, but I will try to find some time to take another crack at this in the not-too-distant future.