Skip to content

feat(dlx): options to skip modules cache#8722

Closed
KSXGitHub wants to merge 6 commits intomainfrom
dlx-resolve-latest
Closed

feat(dlx): options to skip modules cache#8722
KSXGitHub wants to merge 6 commits intomainfrom
dlx-resolve-latest

Conversation

@KSXGitHub
Copy link
Contributor

@KSXGitHub KSXGitHub commented Oct 31, 2024

@KSXGitHub KSXGitHub marked this pull request as ready for review October 31, 2024 10:14
@KSXGitHub KSXGitHub requested a review from zkochan as a code owner October 31, 2024 10:14
@devjiwonchoi
Copy link

devjiwonchoi commented Oct 31, 2024

How about @beta, @canary, or @next?

Can we bypass cache when a dist-tag is given? Use cache only if no @<tag> was given or a valid semver was given.

  • pnpx foo -> use cache

  • pnpx foo@14.0.0 -> use cache

  • pnpx foo@14.0.0-beta.1 -> use cache

  • pnpx foo@14 -> bypass cache

  • pnpx foo@next -> bypass cache

  • pnpx foo@latest -> bypass cache

@KSXGitHub KSXGitHub changed the title feat(dlx): skip modules cache when @latest is specified feat(dlx): options to skip modules cache Oct 31, 2024
@KSXGitHub
Copy link
Contributor Author

@devjiwonchoi Since I am not sure what should be the reasonable expectation for dist tags that aren't "latest", I decide to give the user a choice in the form of a flag.

list: [
{
description: 'Do not use or save the modules cache',
name: '--ignore-modules-cache',
Copy link
Member

Choose a reason for hiding this comment

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

it is a bit long for a CLI option. Maybe just --no-cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fear it would confuse the users. Many would think it ignore all the cache (including the store).

Comment on lines +83 to +92
if (!!opts.ignoreModulesCache || pkgs.some(pkg => pkg.endsWith('@latest'))) {
prepareDir = tempy.directory({ prefix: 'pnpm-dlx-' })
} else {
cache = findCache(pkgs, {
dlxCacheMaxAge: opts.dlxCacheMaxAge,
cacheDir: opts.cacheDir,
registries: opts.registries,
})
prepareDir = cache.dir
}
Copy link
Member

Choose a reason for hiding this comment

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

You probably could just replace the cacheDir with the temp dir. It will do a few redundant filesytem operations but the difference shouldn't be noticeable.

Suggested change
if (!!opts.ignoreModulesCache || pkgs.some(pkg => pkg.endsWith('@latest'))) {
prepareDir = tempy.directory({ prefix: 'pnpm-dlx-' })
} else {
cache = findCache(pkgs, {
dlxCacheMaxAge: opts.dlxCacheMaxAge,
cacheDir: opts.cacheDir,
registries: opts.registries,
})
prepareDir = cache.dir
}
let cacheDir: string
if (!!opts.ignoreModulesCache || pkgs.some(pkg => pkg.endsWith('@latest'))) {
cacheDir = tempy.directory({ prefix: 'pnpm-dlx-' })
} else {
cacheDir = opts.cacheDir
}
const { cacheLink, cacheExists, cachedDir } = findCache(pkgs, {
dlxCacheMaxAge: opts.dlxCacheMaxAge,
cacheDir,
registries: opts.registries,
})
if (!cacheExists) {
fs.mkdirSync(cachedDir, { recursive: true })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5c3021e

(I decided to keep cache.dir instead of cachedDir because cachedDir and cacheDir look too similar to each other, which may cause confusion)

@zkochan
Copy link
Member

zkochan commented Nov 4, 2024

Even if we ignore cache only when the @latest tag is used. I am not sure it improves user experience.

For instance, you run a command with the @latest tag. However, you do a typo in the arguments. You want to get the previously executed command from history, fix the argument and run the command again. But now everything will be installed again even though only a few seconds passed.

We don’t know what most users prefer. We have a discussion with just few user opinions. I am not sure that’s enough to make a decision.

There are a couple of alternative solutions:

  • We could just have a short flag for ignoring the cache. It would work especially well if we’d inform users that cache was used. But unfortunately we were asked in the past to not print anything except of the output of the executed command.
  • We could suggest users to clean the dlx cache when they want to be sure that the packages will be fresh.
  • We could have a shorter cache lifetime for packages specified via range or dist-tag (1 hour or even 5 minutes?)
  • We could resolve packages to exact versions first and then calculate the cache key always by exact versions. This would make the command a little bit slower with cache.

cc @pnpm/collaborators

@devjiwonchoi
Copy link

devjiwonchoi commented Nov 4, 2024

My two cents here:

We could resolve packages to exact versions first and then calculate the cache key always by exact versions. This would make the command a little bit slower with cache.

This matches with my mental model. When I run with a specific dist-tag, I expect it to match with the registry.

Here's one edge case I can think of:

When a user ran pnpx foo@latest, the registry had 1.0.0. An hour later, it turned out that this version had a breaking bug, so it had a hotfix patch 1.1.0. The user was informed to try the "latest" version again, so they ran again with @latest. As it was cached, the package was still on 1.0.0, and they did not know about it until it broke again. The user reported an issue to the foo package, and the maintainers cannot reproduce it on 1.1.0. Since the PNPM docs don't mention the dlx cache, the user couldn't find it until trying pnpx foo@latest --version.

The same goes for @rc, @beta, etc. IMHO, we should at least fetch for the registry when a dist-tag was given and cache by the exact versions.

The options I'd prefer are one of the following:

  1. Cache by exact version, trade-off perf on dlx.
  2. Add an option to bypass cache.

Both options need an update of the dlx documentation about the caching.

@zkochan
Copy link
Member

zkochan commented Nov 26, 2024

The discussed solution: #8811

@zkochan zkochan closed this in 7d7c51e Nov 27, 2024
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.

3 participants