-
Notifications
You must be signed in to change notification settings - Fork 925
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
Conversation
🦋 Changeset detectedLatest commit: 0f0ae10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
8b6a9ba
to
3caf0e2
Compare
3caf0e2
to
c2b3ae0
Compare
function loadViteManifest(directory: string) { | ||
const contents = fs.readFileSync( | ||
path.resolve(directory, ".vite", "manifest.json"), | ||
"utf-8" | ||
); | ||
|
||
return JSON.parse(contents) as vite.Manifest; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
// Remove `assets` field as there are no assets | ||
workerConfig.assets = undefined; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
if (!fs.existsSync(destDir)) { | ||
fs.mkdirSync(destDir, { recursive: true }); | ||
} |
There was a problem hiding this comment.
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?
if (!fs.existsSync(destDir)) { | |
fs.mkdirSync(destDir, { recursive: true }); | |
} | |
fs.mkdirSync(destDir, { recursive: true }); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 |
…uild if there are no assets
365c4da
to
0f0ae10
Compare
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. |
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:
public
directory assetspublic
directory assetsThis 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 outputwrangler.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.