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

Unify cache location configurability #22044 #22047

Closed
wants to merge 0 commits into from
Closed

Conversation

kubijo
Copy link
Contributor

@kubijo kubijo commented Apr 12, 2023

Closes #22044

What I did

Use find-cache-dir in places where the cache location was still constructed somewhat manually.

How to test

Providing a custom cache directory location in a CACHE_DIR environment variable should result in it being used by storybook instead of node_modules/.cache/*

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

const cachePath = join(process.cwd(), 'node_modules', '.cache', 'vite-plugin-externals');
await ensureDir(cachePath);
await emptyDir(cachePath);
const cachePath = findCacheDirectory({ name: 'sb-vite-plugin-externals', create: true }) as string;
Copy link
Contributor Author

@kubijo kubijo Apr 12, 2023

Choose a reason for hiding this comment

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

I've noticed that you have used this sb- prefix notation elsewhere, so I thought to unify it here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also ... I don't much like the string type coercion, but I don't have a clue what would be the correct way of making sure that this passes type safety… as far as the real implementation goes, this should be at the same safety level as before because if the undefined return case is because there might be some problem with access rights, ensureDir & emptyDir would have those too

@@ -51,7 +52,7 @@ export async function commonConfig(

const sbConfig: InlineConfig = {
configFile: false,
cacheDir: 'node_modules/.cache/.vite-storybook',
cacheDir: findCacheDirectory({ name: 'sb-vite' }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto about the dirname pattern unnification

cacheDirectory = path.resolve(projectDir, 'node_modules/.cache/storybook');
}
let cacheDirectory = findCacheDirectory({ name: 'storybook' });
cacheDirectory ||= path.join(process.cwd(), '.cache/storybook');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The findCacheDirectory function's apparently can return undefined and this seemed like the most readable form of a fallback value while also keeping your code-style.

@kubijo
Copy link
Contributor Author

kubijo commented Apr 12, 2023

I have got some trouble with running the sandbox locally even though I have followed the contribution documentation… see https://discord.com/channels/486522875931656193/839297503446695956/1095654464607768646

@kubijo
Copy link
Contributor Author

kubijo commented Apr 13, 2023

I have finally been able to make the yarn start process work again somehow by running:

  • git clean -fX .
  • yarn start again

This is the result of the test:

command ls -a1 sandbox/react-vite-default-ts/node_modules/ | grep '^\.'
.
..
.bin
.yarn-state.yml

❯ echo $CACHE_DIR
/home/pepa/test_cache_loc

❯ tree $CACHE_DIR
 /home/pepa/test_cache_loc
├──  sb-manager
│  ├──  essentials-actions-2
│  │  └──  manager-bundle.mjs
│  ├──  essentials-backgrounds-3
│  │  └──  manager-bundle.mjs
│  ├──  essentials-controls-1
│  │  └──  manager-bundle.mjs
│  ├──  essentials-measure-6
│  │  └──  manager-bundle.mjs
│  ├──  essentials-outline-7
│  │  └──  manager-bundle.mjs
│  ├──  essentials-toolbars-5
│  │  └──  manager-bundle.mjs
│  ├──  essentials-viewport-4
│  │  └──  manager-bundle.mjs
│  ├──  interactions-8
│  │  └──  manager-bundle.mjs
│  └──  links-0
│     └──  manager-bundle.mjs
├──  sb-vite
│  └──  deps
│     └──  …trimmed for brevity, many files here…
├──  sb-vite-plugin-externals
│  └──  @storybook
│     ├──  addons.js
│     ├──  channel-postmessage.js
│     ├──  channel-websocket.js
│     ├──  channels.js
│     ├──  client-api.js
│     ├──  client-logger.js
│     ├──  core-client.js
│     ├──  core-events.js
│     ├──  preview-api.js
│     ├──  preview-web.js
│     └──  store.js
└──  storybook
   ├──  dev-server
   │  ├──  325c8f456729b912b0d2134054eb7448-05dc46265bc37a5127fcc8dfad211dc1
   │  ├──  325c8f456729b912b0d2134054eb7448-5608341ec09d3d75fd946188791fdbd1
   │  └──  325c8f456729b912b0d2134054eb7448-dfeeb2271cc2857eb0a45a5003c8bbee
   └──  public
      └──  sb-addons
         ├──  essentials-actions-2
         │  ├──  manager-bundle.mjs
         │  ├──  manager-bundle.mjs.LEGAL.txt
         │  └──  manager-bundle.mjs.map
         ├──  essentials-backgrounds-3
         │  ├──  manager-bundle.mjs
         │  ├──  manager-bundle.mjs.LEGAL.txt
         │  └──  manager-bundle.mjs.map
         ├──  essentials-controls-1
         │  ├──  manager-bundle.mjs
         │  ├──  manager-bundle.mjs.LEGAL.txt
         │  └──  manager-bundle.mjs.map
         ├──  essentials-measure-6
         │  ├──  manager-bundle.mjs
         │  ├──  manager-bundle.mjs.LEGAL.txt
         │  └──  manager-bundle.mjs.map
         ├──  essentials-outline-7
         │  ├──  manager-bundle.mjs
         │  ├──  manager-bundle.mjs.LEGAL.txt
         │  └──  manager-bundle.mjs.map
         ├──  essentials-toolbars-5
         │  ├──  manager-bundle.mjs
         │  ├──  manager-bundle.mjs.LEGAL.txt
         │  └──  manager-bundle.mjs.map
         ├──  essentials-viewport-4
         │  ├──  manager-bundle.mjs
         │  ├──  manager-bundle.mjs.LEGAL.txt
         │  └──  manager-bundle.mjs.map
         ├──  interactions-8
         │  ├──  manager-bundle.mjs
         │  ├──  manager-bundle.mjs.LEGAL.txt
         │  └──  manager-bundle.mjs.map
         └──  links-0
            ├──  manager-bundle.mjs
            ├──  manager-bundle.mjs.LEGAL.txt
            └──  manager-bundle.mjs.map

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 this pull request may close these issues.

[Feature Request]: Unify the cache locations configurability by using find-cache-dir everywhere
1 participant