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

Enable cache directory path to be set explicitly #3064

Merged
merged 3 commits into from May 20, 2021

Conversation

fwouts
Copy link
Contributor

@fwouts fwouts commented Mar 31, 2021

Changes

This enables the cache directory, which is by default node_modules/.cache/snowpack, to be set explicitly via CLI flag or configuration. This can be useful in situations where, for example, you are running Snowpack for a project that isn't in the current working directory.

Example usage

CLI

snowpack dev --cache-dir-path .snowpack-cache

API

await snowpack.startServer({
  config: snowpack.createConfiguration({
    buildOptions: {
      cacheDirPath: path.join(__dirname, ".snowpack-cache"),
    },
    ...
  }),
  lockfile: null,
});

Testing

This is simply piping through configuration values. Happy to add a test, but I'm not sure it's needed :)

Docs

There doesn't seem to be explicit documentation for every CLI flag at this stage, so no documentation was added.

@vercel
Copy link

vercel bot commented Mar 31, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/DZtXhuAtNyNJZ6JR11y3PcHaYHKE
✅ Preview: https://snowpack-git-fork-fwouts-custom-cache-dir-pikapkg.vercel.app

@FredKSchott
Copy link
Owner

Nice!!! I'm actually just getting ready to merge #3028 which changes some of this behavior. I'd love to add this support after that gets merged (I'll help rebase if there are any conflicts).

@fwouts
Copy link
Contributor Author

fwouts commented Apr 1, 2021

No worries at all, I'm happy to take care of merge conflicts. Good opportunity to get more familiar with the codebase!

@fwouts
Copy link
Contributor Author

fwouts commented Apr 11, 2021

@FredKSchott merge conflict is dealt with :)

@fwouts
Copy link
Contributor Author

fwouts commented Apr 12, 2021

By the way, I noticed that Snowpack is noticeably slower since #3028 was merged. On a relatively simple project (slow-storybook branch in this repo), it went from 24 seconds to prepare the cache on the previous commit (aa1de0b), to 82 seconds (commit 25ed288). I noticed it hasn't been released yet, so it's probably a known issue. Happy to file one otherwise!

Copy link
Collaborator

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

This looks good to me! Only one last request before merging: we actually do have information on CLI flags if you run snowpack --help. Can you add this in there (snowpack/src/index.ts)? Then we can merge & ship this.

@drwpow
Copy link
Collaborator

drwpow commented Apr 29, 2021

Also please add documentation for this here, as well: https://www.snowpack.dev/reference/configuration#buildoptions

// If `projectCacheDir()` is null, no node_modules directory exists.
// Use the current path (hashed) to create a cache entry in the global cache instead.
// Because this is specifically for dependencies, this fallback should rarely be used.
path.join(GLOBAL_CACHE_DIR, crypto.createHash('md5').update(process.cwd()).digest('hex'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this deterministic hash approach 👍🏻

@drwpow
Copy link
Collaborator

drwpow commented Apr 29, 2021

@fwouts I was not aware of that. Taking a look at that reproduction now.

@drwpow
Copy link
Collaborator

drwpow commented Apr 29, 2021

@fwouts you‘re right—the recent changes really do slow down the streaming builds 😕. That’s definitely worth opening up an issue about if you have time to.

@fwouts
Copy link
Contributor Author

fwouts commented Apr 29, 2021

@fwouts you‘re right—the recent changes really do slow down the streaming builds 😕. That’s definitely worth opening up an issue about if you have time to.

Sorry! I no longer use Snowpack so I may not be in the best position to follow up on this 😶 I will of course address the feedback on the current PR to get it in a mergeable state though!

@fwouts fwouts requested a review from a team as a code owner May 8, 2021 02:23
@drwpow drwpow merged commit 84e39f8 into FredKSchott:main May 20, 2021
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.

None yet

3 participants