Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build: Replace globby with fdir #19297

Closed
wants to merge 16 commits into from
Closed

Build: Replace globby with fdir #19297

wants to merge 16 commits into from

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Sep 29, 2022

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 (terkelg/tiny-glob#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
Copy link
Member Author

IanVS commented Sep 29, 2022

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
Copy link
Member Author

IanVS commented Sep 30, 2022

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
Copy link
Member Author

IanVS commented Oct 3, 2022

I've broken out a separate PR to fix the issues with addon-interactions: #19334

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though the code generally looks a bit more complex, I'm ok with it.

But i noticed in a few places we're using .sync(); which seems wasteful because this is going to touch the FS and so be potentially slow & blocking.

@IanVS
Copy link
Member Author

IanVS commented Oct 5, 2022

Waiting for thecodrr/fdir#80 to be released, which should fix yarn pnp.

I'll try making these calls async as well.

@socket-security
Copy link

socket-security bot commented Oct 17, 2022

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 foo@1.0.0 bar@2.4.2

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Powered by socket.dev

@ndelangen
Copy link
Member

Merge when green!

@IanVS
Copy link
Member Author

IanVS commented Oct 18, 2022

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 IanVS marked this pull request as draft October 18, 2022 12:23
@IanVS
Copy link
Member Author

IanVS commented Nov 17, 2022

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 IanVS closed this Nov 17, 2022
@thecodrr
Copy link

thecodrr commented Aug 6, 2023

@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 Add (failing) test for filename glob thecodrr/fdir#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.

@IanVS
Copy link
Member Author

IanVS commented Aug 16, 2023

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 IanVS mentioned this pull request Aug 30, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants