storybook icon indicating copy to clipboard operation
storybook copied to clipboard

Build: Replace globby with fdir

Open IanVS opened this issue 2 years ago • 7 comments

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:

  1. install a polyfill of setImmediate. Seemed like the wrong approach.
  2. 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.
  3. 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.

IanVS avatar Sep 29 '22 18:09 IanVS

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

IanVS avatar Sep 29 '22 18:09 IanVS

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

IanVS avatar Sep 30 '22 17:09 IanVS

I've broken out a separate PR to fix the issues with addon-interactions: https://github.com/storybookjs/storybook/pull/19334

IanVS avatar Oct 03 '22 16:10 IanVS

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.

IanVS avatar Oct 05 '22 21:10 IanVS

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

socket-security[bot] avatar Oct 17 '22 18:10 socket-security[bot]

Merge when green!

ndelangen avatar Oct 18 '22 12:10 ndelangen

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.

IanVS avatar Oct 18 '22 12:10 IanVS

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 avatar Nov 17 '22 00:11 IanVS

@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:

  1. Everything was rewritten in TypeScript
  2. fdir now doesn't resolve symlinks by default (but requires withSymlinks option)
  3. globbing now supports changing picomatch options with the .globWithOptions function. This fixes the behavior you mentioned in https://github.com/thecodrr/fdir/pull/85
  4. 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.

thecodrr avatar Aug 06 '23 11:08 thecodrr

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.

IanVS avatar Aug 16 '23 13:08 IanVS