-
Notifications
You must be signed in to change notification settings - Fork 268
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
Add Readmes for public and app/assets directories #1934
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,7 @@ | |||
Use the `/public` folder to store static files that **shouldn't be processed at build time**. |
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.
This file here will be publicly available in domain.com/readme.md
if they don't remove it manually. Perhaps we should inform about assets in the root template readme instead?
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.
Ha, I wondered about that. Could move it out for sure, although I think having it in context has some value. Is there some way to configure an ignore rule?
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.
Just testing this out, and of course the build process does produce dist/client/readme.md
. But it throws a 404 in Oxygen: https://01htfeq3ddq1kvwp4adw76d3tf-bdc62392032d96aafe24.myshopify.dev/readme.md — wondering if the CDN just ignores certain file types by default or if there's something else going on.
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 the worker domain. I guess it doesn't proxy .md
files to the CDN 🤔
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.
@jgodson confirmed: https://github.com/Shopify/oxygen-workers/blob/fc213b66075fdd59a74954317fa77b4794d91a64/packages/routing-worker/src/assets.ts#L23-L44
@gfscott is the desire to give this folder context on Github with the readme file? Or also to give context inside vscode after you've scaffolded a new project? If it's just Github, we could update the CLI to not copy it over on new projects.
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.
Nm, apparently there's a time in the future where this will change, and our proxy logic will not filter assets like this, so then this file would start to show up.
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.
is the desire to give this folder context on Github with the readme file? Or also to give context inside vscode after you've scaffolded a new project?
More the latter. Why ask them to google it or pore over the docs when we could just say what the folder is, right in context? Seems to me like the canonical role of a readme file.
That said, example.com/readme.md
is not a desirable outcome 😅
Is there some exclude/ignore list we can give to Vite at build time?
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.
I think we'd need to update the deploy command to exclude them somehow. Maybe an ignore file. I don't think we will have time to do this for the release this week.
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.
Oh yeah, this is not urgent at all
Co-authored-by: Fran Dios <[email protected]>
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
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.
I don't think we should ship things to the public directory at the moment, unless we have mechanism to prevent them from getting deployed or copied in the starter template.
We've observed some confusion in the wild about the two directories where Hydrogen projects can keep static assets:
/public
and/app/assets
.This PR adds a readme to each directory with some contextual detail on what happens to the assets stored there. Very open to edits, fact-checks, etc.
Checklist
I've added tests to cover my changesI've added or updated the documentation