Skip to content

feat: generate .assetsignore file for Cloudflare deployment #13109

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.

Sign up for GitHub

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 15 commits into from
Dec 19, 2024
Merged

feat: generate .assetsignore file for Cloudflare deployment #13109

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
14b662f
.assetsignore file generation for Cloudflare deployment. It is corres…
methanoya Dec 5, 2024
32ccc2e
move .assetsignore to files folder
methanoya Dec 7, 2024
0e9b1fc
add .assetsignore file generation for adapter-cloudflare-workers
methanoya Dec 7, 2024
ee1e4bb
changeset update
methanoya Dec 7, 2024
6892797
move .gitignore per Ben McCann (benmccann) request
methanoya Dec 9, 2024
9061fc5
Update .changeset/proud-taxis-admire.md
teemingc Dec 10, 2024
1259966
Update .changeset/proud-taxis-admire.md
methanoya Dec 10, 2024
ae98415
Revert "add .assetsignore file generation for adapter-cloudflare-work…
methanoya Dec 12, 2024
38a0220
merge user's .assetsignore with the generated one
methanoya Dec 12, 2024
896318c
Update proud-taxis-admire.md
methanoya Dec 12, 2024
3b27f87
add ending newline to the generated .assetsignore
methanoya Dec 12, 2024
99d2fd9
move `.assetsignore` generation to generate_assetsignore()
methanoya Dec 17, 2024
a1ec57a
simplify the change
methanoya Dec 18, 2024
728fb08
cleanup unused import
methanoya Dec 18, 2024
68bdb25
Update .changeset/proud-taxis-admire.md
teemingc Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/adapter-cloudflare-workers/files/.assetsignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
_worker.js
_routes.json
_headers
_redirects
4 changes: 3 additions & 1 deletion packages/adapter-cloudflare-workers/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { existsSync, readFileSync, writeFileSync } from 'node:fs';
import { copyFileSync, existsSync, readFileSync, writeFileSync } from 'node:fs';
import { posix, dirname } from 'node:path';
import { execSync } from 'node:child_process';
import esbuild from 'esbuild';
Expand Down Expand Up @@ -143,6 +143,8 @@ export default function ({ config = 'wrangler.toml', platformProxy = {} } = {})
);
}

copyFileSync(`${files}/.assetsignore`, `${dirname(main)}/.assetsignore`);
Copy link
Member

@teemingc teemingc Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@teemingc teemingc Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@methanoya methanoya Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@teemingc teemingc Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.


builder.log.minor('Copying assets...');
const bucket_dir = `${site.bucket}${builder.config.kit.paths.base}`;
builder.writeClient(bucket_dir);
Expand Down
1 change: 0 additions & 1 deletion packages/adapter-cloudflare/.gitignore
View file
Open in desktop

This file was deleted.

4 changes: 4 additions & 0 deletions packages/adapter-cloudflare/files/.assetsignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
_worker.js
_routes.json
_headers
_redirects
2 changes: 2 additions & 0 deletions packages/adapter-cloudflare/files/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
*
!.assetsignore
4 changes: 3 additions & 1 deletion packages/adapter-cloudflare/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { existsSync, writeFileSync } from 'node:fs';
import { copyFileSync, existsSync, writeFileSync } from 'node:fs';
import * as path from 'node:path';
import { fileURLToPath } from 'node:url';
import * as esbuild from 'esbuild';
Expand Down Expand Up @@ -74,6 +74,8 @@ export default function (options = {}) {
});
}

copyFileSync(`${files}/.assetsignore`, `${dest}/.assetsignore`);

builder.copy(`${files}/worker.js`, `${tmp}/_worker.js`, {
replace: {
SERVER: `${relativePath}/index.js`,
Expand Down
Loading