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

feat: persistent cache plugin #14333

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

Akryum
Copy link
Contributor

@Akryum Akryum commented Sep 8, 2023

Description

Continuation of #10671

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented Sep 8, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@Akryum
Copy link
Contributor Author

Akryum commented Sep 8, 2023

We would still need a way to handle plugin's load with side effects such as in svelte's plugin, see https://github.com/vitejs/vite/pull/10671/files#r1010031224

Copy link
Member

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

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

Thanks for this new proposal which is very aligned with what @yyx990803 suggested.

High level review that requires approval from the rest of the team:

  • The callbacks should be calculated in advanced which will be better for perf
  • The key&read callbacks should be merged in one callback
    • This simplify the code
    • This don't impose a string key to the plugin authors
    • This allow the key to be async
  • Because the read cb is async, to avoid to many empty ticks if there is no "cache plugin", the plugin container should expose an optional cb

@Akryum
Copy link
Contributor Author

Akryum commented Sep 13, 2023

The callbacks should be calculated in advanced which will be better for perf

Isn't getSortedPlugins already cached?

"cache"
],
"main": "./dist/index.cjs",
"module": "./dist/index.mjs",
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to include module. It will be ignored since you've defined exports

Suggested change
"module": "./dist/index.mjs",

"vite-plugin",
"cache"
],
"main": "./dist/index.cjs",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest adding "type": "module" and only distributing an ESM version since this is a new plugin. Vite is working on dropping the CJS versions of its builds - they're deprecated in v5 and will be removed in v6

@@ -0,0 +1,18 @@
# @vitejs/plugin-persistent-cache [![npm](https://img.shields.io/npm/v/@vitejs/plugin-persistent-cache.svg)](https://npmjs.com/package/@vitejs/plugin-persistent-cache)

Copy link
Collaborator

@benmccann benmccann Oct 29, 2023

Choose a reason for hiding this comment

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

this could use some more docs:

  • what exactly is being cached?
  • how is invalidation handled?
  • i'm assuming it applies to the results of other plugins as well?
  • does it apply in both dev and prod?
  • can you control what directory the info is persisted in?
  • is there anything you need to do to use this on GitHub Actions?
  • the options filled out below

packages/plugin-persistent-cache/package.json Outdated Show resolved Hide resolved
import fs from 'node:fs'
import path from 'node:path'

export function getCodeHash(code: string): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could probably just be called getHash. it will work regardless of whether the input is a "code" or not

return createHash('sha1').update(code).digest('hex')
}

export function tryStatSync(file: string): fs.Stats | undefined {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export function tryStatSync(file: string): fs.Stats | undefined {
function tryStatSync(file: string): fs.Stats | undefined {

}
}

export function lookupFile(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export function lookupFile(
/** returns the first matching file path of the given file names */
export function lookupFile(

"require": "./dist/index.cjs"
}
},
"scripts": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing we need to copy the lint, format, and typecheck from the main Vite package to be here as well

const loadResult = await pluginContainer.load(id, { ssr })
let loadResult: LoadResult

const cacheLoadResult = await pluginContainer.serveLoadCacheRead({
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like this happens in both dev and build in which case I don't think we should call it "serve" because it's a bit confusing with Vite's "serve" command (https://vitejs.dev/config/#conditional-config)

maybe we could call it something like getFromLoadCache

])
if (packageLockFile) {
cacheVersionFromFiles.push(packageLockFile)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we throw an exception if no package lock file is found? I think that would only happen if the user were using a different package manager and the caching logic might not be safe in that case

@benmccann
Copy link
Collaborator

benmccann commented Oct 30, 2023

I don't know if this would be considered out-of-scope, but it would be nice to have a way to cache generated assets, as it can be extremely expensive. E.g. there are requests for caching assets in JonasKruckenberg/imagetools#522, sveltejs/kit#11043, and zerodevx/svelte-img#35.

In vite-imagetools we currently do the following:

const fileHandle = this.emitFile({ type: 'asset' })
metadata.src = `__VITE_ASSET__${fileHandle}__`

And actually, I wonder if this plugin would break for projects with vite-imagetools installed if we don't have such support because otherwise I assume it's going to be referring to files that don't exist.

@benmccann
Copy link
Collaborator

benmccann commented Nov 20, 2023

We would still need a way to handle plugin's load with side effects such as in svelte's plugin, see https://github.com/vitejs/vite/pull/10671/files#r1010031224

I don't think we should worry about supporting side effects generally. I'd imagine we could update vite-plugin-svelte to use these new persistent cache hooks rather than its own transient in memory cache implementation. @dominikg can correct me if this wouldn't be feasible.

However, I do think we should support the emitFile side-effect mentioned in my last comment since it's a core part of Vite/Rollup.

@patak-dev patak-dev added the p2-to-be-discussed Enhancement under consideration (priority) label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Status: Later
Development

Successfully merging this pull request may close these issues.

4 participants