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 11 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
5 changes: 5 additions & 0 deletions .changeset/proud-taxis-admire.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/adapter-cloudflare': minor
---

feat: add `.assetsignore` file to avoid serving known server-only files to browser
3 changes: 2 additions & 1 deletion packages/adapter-cloudflare/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
/files
/files/*
!/files/.assetsignore
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
16 changes: 15 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 { existsSync, readFileSync, writeFileSync } from 'node:fs';
import * as path from 'node:path';
import { fileURLToPath } from 'node:url';
import * as esbuild from 'esbuild';
Expand Down Expand Up @@ -143,6 +143,20 @@ export default function (options = {}) {
}`
);
}

const assetsignore = [`${dest}/.assetsignore`, `${files}/.assetsignore`].reduce(
(acc, file) => {
if (existsSync(file)) {
readFileSync(file, 'utf8')
.split('\n')
.filter((line) => line.trim())
.forEach((line) => acc.add(line));
}
return acc;
},
new Set()
);
writeFileSync(`${dest}/.assetsignore`, Array.from(assetsignore).sort().join('\n') + '\n');
Copy link
Member

@teemingc teemingc Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd recommend appending to the existing file instead, similar to the generate_headers function. It should be fine if we have duplicate entries. This way we don't need the files/.assetsignore file and .gitignore change either.

Suggested change
const assetsignore = [`${dest}/.assetsignore`, `${files}/.assetsignore`].reduce(
(acc, file) => {
if (existsSync(file)) {
readFileSync(file, 'utf8')
.split('\n')
.filter((line) => line.trim())
.forEach((line) => acc.add(line));
}
return acc;
},
new Set()
);
writeFileSync(`${dest}/.assetsignore`, Array.from(assetsignore).sort().join('\n') + '\n');
writeFileSync(`${dest}/.assetsignore`, generate_assetsignore(), { flag: 'a' }); 
 function generate_assetsignore() { 
 	return ` 
_worker.js
_routes.json
_headers
_redirects
 `.trimEnd(); 
 }

Copy link
Contributor Author

@methanoya methanoya Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have simplified the code and added generate_assetsignore() function to handle the .assetsignore separately. It is not clear why keeping the source code intact (.gitignore and files) would take priority over avoiding the hardcoding. The .assetsignore comes from another project cloudflare/workers-sdk and it makes sense to keep it separated for readability and maintainability in case of further updates.
For now, the generate_assetsignore() simply merges the user's .assetsignore file and the original one into the final .assetsignore. The proposed snipped above doesn't provide the merging, so cannot be used as is.

Copy link
Member

Choose a reason for hiding this comment

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

The proposed snipped above doesn't provide the merging, so cannot be used as is.

It does merge it because if the user has the file /static/.assetsignore it would have been copied over to the destination and those entries would be appended to the end of the existing file or write to a new file if it doesn't already exist.

Copy link
Contributor Author

@methanoya methanoya Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, I missed the first part of the update besides the snippet. It looks like my concerns are ignored, so I've updated the PR as suggested but added comments about the source of the .assetsignore.

},
async emulate() {
const proxy = await getPlatformProxy(options.platformProxy);
Expand Down
Loading