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

Investigate migrating away from file-system-cache #29168

Closed
Tracked by #29038
JReinhold opened this issue Sep 20, 2024 · 1 comment · Fixed by #29256
Closed
Tracked by #29038

Investigate migrating away from file-system-cache #29168

JReinhold opened this issue Sep 20, 2024 · 1 comment · Fixed by #29256
Assignees

Comments

@JReinhold
Copy link
Contributor

JReinhold commented Sep 20, 2024

file-system-cache has some problems:

  1. It's a pretty big dependency for just reading and writing to the file system in a specific directory (we don't even use it to specify the directory, we use find-cache-dir for that, which is also sort of dubious).
  2. It includes all of ramda (lodash-like lib) because it imports it with a wildcard.
  3. We've had some typing issues with it before, where it was distributed without the necessary sub-type packages.
  4. The versioning/releasing is funky, as currently the versions 3.0.0-alpha.0-8 have been released after 3.0.0, which doesn't make any sense. Furthermore, 3.0.0-alpha.8 is currently the version on the latest tag: https://www.npmjs.com/package/file-system-cache?activeTab=versions

Problem 2-4 could probably be fixed upstream if we wanted instead.

It provides a nice and simple get/set API around reading/writing to a directory. It might do more, but that is all we use it for, and to me it seems like something we could easily do internally without the problems listed above. As far as I can see it doesn't do anything special to improve the perf, it just uses node:fs and fs-extra under the hood.

@JReinhold
Copy link
Contributor Author

@TheThing made a great alternative with https://git.nfp.is/TheThing/fs-cache-fast 🙏, that we could use either directly, or just copying the code as it's so tiny.

A few thoughts on initial inspection:

  1. It doesn't come with type definitions because it's written in JS. This doesn't work for us, we'd either need to write a separate .d.ts file, or convert it to ts, either internally in Storybook or in the package.
  2. All the imports from Node-natives (like os) should have the node: prefix to ensure a rouge package doesn't take over the import. 'os' -> 'node:os'
  3. It would be preferable if the code was hosted on GitHub as that would lower the friction for contributions (and thus maintenance), but I also don't know the reasons for using a self-hosted Gitea, there could be valid reasons that I don't know about.

But it has great test coverage, which looks good.

@TheThing are you still interested in opening a PR with this, or should we attempt a migration in the core team?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants