feat: generate `.assetsignore` file for Cloudflare deployment #13109

methanoya · 2024-12-05T00:29:17Z

It is corresponded to cloudflare/workers-sdk#6640

Resolves #13108

This PR avoids Cloudflare deployment errors like Uploading a Pages _worker.js file as an asset by adding .assetsignore file generation.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
~~Ideally, include a test that fails without this PR but passes with it.~~ These adapters don't appear to have tests.

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

…ponded to cloudflare/workers-sdk#6640

packages/adapter-cloudflare/package.json

benmccann · 2024-12-06T20:46:06Z

And feature added to the cloudflare adapter must be added to the cloudflare workers adapter as well

changeset-bot · 2024-12-07T00:36:38Z

🦋 Changeset detected

Latest commit: 3b27f87

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/adapter-cloudflare	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

methanoya · 2024-12-07T00:41:15Z

And feature added to the cloudflare adapter must be added to the cloudflare workers adapter as well

I was not completely sure that the adapter-cloudflare-workers is affected but it is updated for now.

packages/adapter-cloudflare/files/.gitignore

eltigerchino · 2024-12-10T01:40:09Z

I misread the original issue. Does this only affect the workers adapter since it's a workers only feature? https://developers.cloudflare.com/workers/static-assets/binding/#ignoring-assets

.changeset/proud-taxis-admire.md

eltigerchino · 2024-12-10T01:54:20Z

packages/adapter-cloudflare-workers/index.js

@@ -143,6 +143,8 @@ export default function ({ config = 'wrangler.toml', platformProxy = {} } = {})
 				);
 			}

+			copyFileSync(`${files}/.assetsignore`, `${dirname(main)}/.assetsignore`);


Seems like we would need #13072 to be merged first, then copy the .assetsignore file to the root of the assets folder.

https://developers.cloudflare.com/workers/static-assets/binding/#ignoring-assets

In this case, create a .assetsignore file in the root of the assets directory.

I'm also wondering if we would want to simply append / write to a file instead of copying so that a user's .assetsignore file would be taken into consideration. Similar to

kit/packages/adapter-cloudflare/index.js

Line 69 in 01d2720

writeFileSync(`${dest}/_headers`, generate_headers(builder.getAppPath()), { flag: 'a' });

where we append the generated _headers to the file.

It looks like #13072 is in a draft state, this PR is ready to use.
The source of the .assetsignore is https://github.com/cloudflare/workers-sdk/blob/main/packages/create-cloudflare/templates-experimental/svelte/js/static/.assetsignore They created it some way, so it makes sense to keep it the same and copy from there if updated. It is a better point to control the list of server-side-only files. Another approach is adding files like _worker.js to the .assetsignore once they are created, but it is probably less clear. Let me know if you have a strong opinion, I will update the PR accordingly.

The difference is that Cloudflare's .assetsignore file is part of a starting template, whereas we need to take into account a user having their own file. I'd recommend that we write using append mode so it's created if it doesn't exist, otherwise it writes to the existing file.

I'm also not sure copying it to main corresponds to the root of the assets directory. Isn't it currently site.bucket?

kit/packages/adapter-cloudflare-workers/index.js

Lines 146 to 149 in 33600ee

builder.log.minor('Copying assets...');

const bucket_dir = `${site.bucket}${builder.config.kit.paths.base}`;

builder.writeClient(bucket_dir);

builder.writePrerendered(bucket_dir);

But I'm also not sure if copying it to the bucket root would have any effect since it's part of the old Workers sites feature. cc: @petebacondarwin

What about a shipping update for adapter-cloudflare only since it would work differently for adapter-cloudflare-workers and can depend on #13072 ?

Making .assetsignore based on an existing one can be a good idea, but it looks like adapter-cloudflare takes responsibility for making the right bundle for the Cloudflare deployment. I am not sure that a user can have a reason to make any non-asset file.
We can show a warning message if .assetsignore is already present as a current workaround.
I am open to both approaches if maintainers strongly believe either way.

What about a shipping update for adapter-cloudflare only since it would work differently for adapter-cloudflare-workers and can depend on #13072 ?

Yeah, I think we can probably split the Workers part into another PR.

Making .assetsignore based on an existing one can be a good idea, but it looks like adapter-cloudflare takes responsibility for making the right bundle for the Cloudflare deployment. I am not sure that a user can have a reason to make any non-asset file. We can show a warning message if .assetsignore is already present as a current workaround. I am open to both approaches if maintainers strongly believe either way.

The main purpose of the file is to prevent certain assets from being made public. This can be desirable if the user has server only assets. I think we should append to an existing .assetsignore file

@eltigerchino I have removed the -workers code from the PR and added .assetsignore merging. It may be overcomplicated, and we would need to add just one file to another. Let me know your thoughts.

methanoya · 2024-12-10T02:04:21Z

I misread the original issue. Does this only affect the workers adapter since it's a workers only feature? https://developers.cloudflare.com/workers/static-assets/binding/#ignoring-assets

It definitely affects adapter-cloudflare since wrangler blocks the deployment with the adapter without the fix or workaround. See #13108. I am not completely sure about -worker. I would suggest to put pkg:adapter-cloudflare label back for both PR and the issue reported

Co-authored-by: Tee Ming <[email protected]>

…ers" This reverts commit 0e9b1fc.

remove mention of the unaffected package

.assetsignore file generation for Cloudflare deployment. It is corres…

14b662f

…ponded to cloudflare/workers-sdk#6640

eltigerchino added the pkg:adapter-cloudflare label Dec 5, 2024

benmccann reviewed Dec 6, 2024

View reviewed changes

packages/adapter-cloudflare/package.json Outdated Show resolved Hide resolved

benmccann changed the title ~~.assetsignore file generation for Cloudflare deployment. It is corres…~~ feat: generate .assetsignore file for Cloudflare deployment Dec 6, 2024

methanoya added 2 commits December 6, 2024 19:18

move .assetsignore to files folder

32ccc2e

add .assetsignore file generation for adapter-cloudflare-workers

0e9b1fc

changeset update

ee1e4bb

benmccann reviewed Dec 7, 2024

View reviewed changes

packages/adapter-cloudflare/files/.gitignore Outdated Show resolved Hide resolved

methanoya requested a review from benmccann December 7, 2024 01:07

move .gitignore per Ben McCann (benmccann) request

6892797

eltigerchino added pkg:adapter-cloudflare-workers and removed pkg:adapter-cloudflare labels Dec 10, 2024

eltigerchino reviewed Dec 10, 2024

View reviewed changes

.changeset/proud-taxis-admire.md Outdated Show resolved Hide resolved

eltigerchino reviewed Dec 10, 2024

View reviewed changes

.changeset/proud-taxis-admire.md Outdated Show resolved Hide resolved

eltigerchino reviewed Dec 10, 2024

View reviewed changes

Update .changeset/proud-taxis-admire.md

9061fc5

eltigerchino added the pkg:adapter-cloudflare label Dec 10, 2024

Update .changeset/proud-taxis-admire.md

1259966

Co-authored-by: Tee Ming <[email protected]>

methanoya requested a review from eltigerchino December 10, 2024 21:51

methanoya and others added 4 commits December 12, 2024 09:10

Revert "add .assetsignore file generation for adapter-cloudflare-work…

ae98415

…ers" This reverts commit 0e9b1fc.

merge user's .assetsignore with the generated one

38a0220

Update proud-taxis-admire.md

896318c

remove mention of the unaffected package

add ending newline to the generated .assetsignore

3b27f87

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generate `.assetsignore` file for Cloudflare deployment #13109

feat: generate `.assetsignore` file for Cloudflare deployment #13109

methanoya commented Dec 5, 2024 •

edited by eltigerchino

Loading

benmccann commented Dec 6, 2024

changeset-bot bot commented Dec 7, 2024 •

edited

Loading

methanoya commented Dec 7, 2024

eltigerchino commented Dec 10, 2024 •

edited

Loading

eltigerchino Dec 10, 2024 •

edited

Loading

methanoya Dec 10, 2024

eltigerchino Dec 11, 2024 •

edited

Loading

methanoya Dec 11, 2024

methanoya Dec 11, 2024 •

edited

Loading

eltigerchino Dec 12, 2024 •

edited

Loading

methanoya Dec 12, 2024

methanoya commented Dec 10, 2024 •

edited

Loading

	builder.log.minor('Copying assets...');
	const bucket_dir = `${site.bucket}${builder.config.kit.paths.base}`;
	builder.writeClient(bucket_dir);
	builder.writePrerendered(bucket_dir);

feat: generate .assetsignore file for Cloudflare deployment #13109

Are you sure you want to change the base?

feat: generate .assetsignore file for Cloudflare deployment #13109

Conversation

methanoya commented Dec 5, 2024 • edited by eltigerchino Loading

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Changesets

Edits

benmccann commented Dec 6, 2024

changeset-bot bot commented Dec 7, 2024 • edited Loading

🦋 Changeset detected

methanoya commented Dec 7, 2024

eltigerchino commented Dec 10, 2024 • edited Loading

eltigerchino Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

methanoya Dec 10, 2024

Choose a reason for hiding this comment

eltigerchino Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

methanoya Dec 11, 2024

Choose a reason for hiding this comment

methanoya Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

eltigerchino Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

methanoya Dec 12, 2024

Choose a reason for hiding this comment

methanoya commented Dec 10, 2024 • edited Loading

feat: generate `.assetsignore` file for Cloudflare deployment #13109

feat: generate `.assetsignore` file for Cloudflare deployment #13109

methanoya commented Dec 5, 2024 •

edited by eltigerchino

Loading

changeset-bot bot commented Dec 7, 2024 •

edited

Loading

eltigerchino commented Dec 10, 2024 •

edited

Loading

eltigerchino Dec 10, 2024 •

edited

Loading

eltigerchino Dec 11, 2024 •

edited

Loading

methanoya Dec 11, 2024 •

edited

Loading

eltigerchino Dec 12, 2024 •

edited

Loading

methanoya commented Dec 10, 2024 •

edited

Loading