Skip to content
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

Jump to bottom

Add Readmes for public and app/assets directories #1934

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add Readmes for public and app/assets directories #1934

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3935757
Add readmes for public and app/assets directories
gfscott Apr 1, 2024
37a29aa
Apply suggestions from code review
gfscott Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions templates/skeleton/app/assets/readme.md
View file
Open in desktop
@@ -0,0 +1,11 @@
Use the `/app/assets` folder to store static files that **should be processed at build time**.

Files in `/app/assets`:
- Get uploaded to the Shopify CDN on deployment
- `/app/assets/logo.png` → `cdn.shopify.com/0000/0000/0000/assets/logo.png`
gfscott marked this conversation as resolved.
Show resolved Hide resolved
- Can be imported in your app files
- Example: `import logo from '~/app/assets/logo.png'`
- Get processed by Hydrogen's build tools
- File names are likely to be hashed (`/app/assets/logo.svg` -> `/dist/assets/logo-p7f8c0gh.svg`)
- SVG images may be inlined
- CSS or JavaScript files may be chunked, uglified, and/or minified
Empty file removed templates/skeleton/public/.gitkeep
View file
Open in desktop
Empty file.
7 changes: 7 additions & 0 deletions templates/skeleton/public/readme.md
View file
Open in desktop
@@ -0,0 +1,7 @@
Use the `/public` folder to store static files that **shouldn't be processed at build time**.
Copy link
Contributor

@frandiox frandiox Apr 2, 2024 •
edited

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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 🤔

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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


Files in `/public`:
- Get uploaded to the Shopify CDN on deployment
- `/public/image.png` → `cdn.shopify.com/0000/0000/0000/image.png`
- Can't be imported in your app files (see `/app/assets` instead)
- Aren't processed by Hydrogen's build tools