Skip to content

Full build support for assets #9510

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

Merged
merged 16 commits into from
Jun 11, 2025
Merged

Full build support for assets #9510

merged 16 commits into from
Jun 11, 2025

Conversation

jamesopstad
Copy link
Contributor

Fixes #9384.

Assets that are imported in the entry Worker are now automatically moved to the client build output. Additionally, this PR adds support for a broader range of build scenarios. These are:

  • Assets only build with client entry/entries
  • Assets only build with no client entry/entries that includes public directory assets
  • Worker(s) + assets build with client entry/entries
  • Worker(s) + assets build with no client entry/entries that includes imported and/or public directory assets
  • Worker(s) build with no assets

This PR switches the build order so that Workers are built first. It also changes the strategy so that the assets field is always populated in the output wrangler.json and is then removed if there are no assets.

Note: for Vite 7 we will reuse some of the logic introduced here in the new buildApp hook.


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • Wrangler / Vite E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: to be updated once static routing is added
  • Wrangler V3 Backport
    • TODO (before merge)
    • Wrangler PR:
    • Not necessary because: not a Wrangler change

@jamesopstad jamesopstad requested a review from a team as a code owner June 6, 2025 12:40
@jamesopstad jamesopstad added the e2e Run wrangler + vite-plugin e2e tests on a PR label Jun 6, 2025
@jamesopstad jamesopstad requested a review from a team as a code owner June 6, 2025 12:40
Copy link

changeset-bot bot commented Jun 6, 2025

🦋 Changeset detected

Latest commit: 0f0ae10

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/vite-plugin Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Jun 6, 2025
@jamesopstad jamesopstad added the vite-plugin Relating to the `@cloudflare/vite-plugin` package label Jun 6, 2025
Copy link

pkg-pr-new bot commented Jun 6, 2025

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@9510

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@9510

miniflare

npm i https://pkg.pr.new/miniflare@9510

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@9510

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@9510

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@9510

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@9510

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@9510

wrangler

npm i https://pkg.pr.new/wrangler@9510

commit: 0f0ae10

@jamesopstad jamesopstad force-pushed the james/assets-build branch from 8b6a9ba to 3caf0e2 Compare June 6, 2025 12:43
@jamesopstad jamesopstad added c3-e2e Run c3 e2e tests on a PR every-os Run tests (unit/e2e/c3-e2e) on every OS, rather than just macOS labels Jun 6, 2025
@jamesopstad jamesopstad force-pushed the james/assets-build branch from 3caf0e2 to c2b3ae0 Compare June 6, 2025 15:58
Comment on lines +161 to +164
function loadViteManifest(directory: string) {
const contents = fs.readFileSync(
path.resolve(directory, ".vite", "manifest.json"),
"utf-8"
);

return JSON.parse(contents) as vite.Manifest;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's been some discussion about accessing the build manifests without reading and writing files and I've opened an issue in the Vite repo for this - vitejs/vite#20052.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Cool. Are there any security concerns about the imported server-side asset being moved to client-side assets which could then potentially be accessed from the browser?

Comment on lines +70 to +71
// Remove `assets` field as there are no assets
workerConfig.assets = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly this is only a problem because the wrangler.jsonc in the assets playground has the following:

"assets": { "binding": "ASSETS" }

and we need to remove that because there is no point in having a binding if there are no assets to binding to.

Is that correct?

In the real world, if there are no assets then the user would not add this field in the first place, right? If so, then should this actually be a user error?

Or am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not quite it. I've changed the implementation in this PR so that the assets field is always included in the output wrangler.json at build time, regardless of whether there are assets or not. It is then removed here if there are no assets. This makes things a lot simpler and removes the need for the client environment to be built first. In Vite 7 we will also be able to add this check in the buildApp hook and determine whether another plugin has built the client environment.

Comment on lines 98 to 100
if (!fs.existsSync(destDir)) {
fs.mkdirSync(destDir, { recursive: true });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might well just be quicker to do?

Suggested change
if (!fs.existsSync(destDir)) {
fs.mkdirSync(destDir, { recursive: true });
}
fs.mkdirSync(destDir, { 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.

fs.mkdirSync(destDir, { recursive: true });
}

fs.renameSync(src, dest);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be safer to just copy rather than move?
Is it possible that the file needs to be in the bundle too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There shouldn't be any need for these files to be in the Worker bundle and I think it's nicer to inspect a clean output directory. We also don't want these files to be inadvertently matched by rules and uploaded as additional modules.

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Jun 9, 2025
@jamesopstad
Copy link
Contributor Author

Cool. Are there any security concerns about the imported server-side asset being moved to client-side assets which could then potentially be accessed from the browser?

Only if this doesn't meet user expectations. I did wonder if we should be blocking direct access to assets that are only imported on the server. We could potentially do this by adding the paths to the run_worker_first array once support for that is added. Might be a nice feature. One issue is that there is a limit on the number of rules and we would be adding to the user's rules so this might be unexpected. If we added each file to the array then it's quite possible we could hit this limit.

@jamesopstad
Copy link
Contributor Author

Cool. Are there any security concerns about the imported server-side asset being moved to client-side assets which could then potentially be accessed from the browser?

Only if this doesn't meet user expectations. I did wonder if we should be blocking direct access to assets that are only imported on the server. We could potentially do this by adding the paths to the run_worker_first array once support for that is added. Might be a nice feature. One issue is that there is a limit on the number of rules and we would be adding to the user's rules so this might be unexpected. If we added each file to the array then it's quite possible we could hit this limit.

Discussed this further with @petebacondarwin. The conclusion was to merge this as is and clearly document how to make assets imported in a Worker private. If, in the future, there is a need for specifying that individual asset imports should be private then we could add a new import attribute for this e.g. with { cloudflare: 'asset' }.

@jamesopstad jamesopstad added this pull request to the merge queue Jun 11, 2025
@jamesopstad jamesopstad removed this pull request from the merge queue due to a manual request Jun 11, 2025
@jamesopstad jamesopstad added this pull request to the merge queue Jun 11, 2025
Merged via the queue into main with commit 590d69b Jun 11, 2025
36 of 41 checks passed
@jamesopstad jamesopstad deleted the james/assets-build branch June 11, 2025 18:10
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Jun 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c3-e2e Run c3 e2e tests on a PR e2e Run wrangler + vite-plugin e2e tests on a PR every-os Run tests (unit/e2e/c3-e2e) on every OS, rather than just macOS vite-plugin Relating to the `@cloudflare/vite-plugin` package
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow Vite plugin to build server first, client last
2 participants