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.

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

feat(vite-plugin): set assets path in build to /assets #5745

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

wmertens
Copy link
Member

Right now, if rollupOptions.output.assetFilenames is not defined, assets will be output as build/q-[hash].ext.

This PR changes that default setting to assets/[hash].[ext].

The reasoning is that for post-processing like $localize that copies the build files into /build/[locale]/, it makes no sense to have to copy .css files and others, especially since all css files will be automatically added to the head and so you'll get a per-language copy of the css file.

This is a slightly breaking change, in case some custom scripting is expecting all files to live under /build. (Seems unlikely though)

Copy link

netlify bot commented Jan 21, 2024

Deploy Preview for qwik-insights ready!

Name Link
🔨 Latest commit 9ec0384
🔍 Latest deploy log https://app.netlify.com/sites/qwik-insights/deploys/65bf5c5c781d5a00081cdf35
😎 Deploy Preview https://deploy-preview-5745--qwik-insights.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

cloudflare-workers-and-pages bot commented Jan 21, 2024

Deploying qwik-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 70cfaaa
Status: ✅  Deploy successful!
Preview URL: https://dcbf3e14.qwik-8nx.pages.dev
Branch Preview URL: https://build-assets-path.qwik-8nx.pages.dev

View logs

@PatrickJS
Copy link
Member

oh I actually need this change haha

@wmertens
Copy link
Member Author

@PatrickJS you can configure this for yourself, it just changes the default

@PatrickJS
Copy link
Member

yeah I see that now 😂 I'll update on my end. plus I also want to keep the names for the assets the same for SEO. It's good to fix this as the default

@PatrickJS
Copy link
Member

@wmertens I want to move all assets to another folder. so dist/client and dist/assets and dist/server

the idea is that I may want to handle assets differently or uploaded somewhere else. I can configure dist/client and dist/server

@PatrickJS
Copy link
Member

for SEO reasons you want to keep the name of the file so I changed it to this for my app

assetFileNames: 'assets/[hash]/[name][extname]'

it also helps to see which fonts are loading in

@PatrickJS
Copy link
Member

for images we want to keep the image name for seo so assets/[hash]/[name][extname] is a better way to include the hash for caching. we also need to make sure the headers are set to immutable for /assets like we do or /build

@wmertens
Copy link
Member Author

wmertens commented May 1, 2024

for images we want to keep the image name for seo so assets/[hash]/[name][extname]

how about assets/[hash]-[name][extname] so that we don't create a zillion directories?

@PatrickJS
Copy link
Member

@wmertens yup that works

@PatrickJS
Copy link
Member

I updated the pr

@PatrickJS
Copy link
Member

for v1 can we do
assetFileNames: 'build/assets/[hash]-[name][extname]'

Copy link

changeset-bot bot commented Jul 30, 2024

🦋 Changeset detected

Latest commit: f3c69ac

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

This PR includes changesets to release 4 packages
Name Type
@builder.io/qwik Minor
eslint-plugin-qwik Minor
@builder.io/qwik-city Minor
create-qwik 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

Copy link
Contributor

github-actions bot commented Jul 30, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
qwik-docs ✅ Ready (View Log) Visit Preview f3c69ac

@wmertens wmertens force-pushed the build-assets-path branch 2 times, most recently from 945566d to 404e4ab Compare July 30, 2024 22:27
Copy link

pkg-pr-new bot commented Jul 30, 2024

commit: f3c69ac

npm i https://pkg.pr.new/@builder.io/qwik@5745
npm i https://pkg.pr.new/@builder.io/qwik-city@5745
npm i https://pkg.pr.new/eslint-plugin-qwik@5745
npm i https://pkg.pr.new/create-qwik@5745

Open in Stackblitz

@wmertens wmertens enabled auto-merge (squash) August 1, 2024 10:20
@wmertens wmertens merged commit 80abcff into main Aug 1, 2024
20 checks passed
@wmertens wmertens deleted the build-assets-path branch August 1, 2024 10:20
This was referenced Aug 1, 2024
maiieul added a commit to maiieul/qwik that referenced this pull request Aug 26, 2024
maiieul added a commit to maiieul/qwik that referenced this pull request Aug 27, 2024
maiieul added a commit to maiieul/qwik that referenced this pull request Aug 27, 2024
maiieul added a commit to maiieul/qwik that referenced this pull request Aug 27, 2024
maiieul added a commit to maiieul/qwik that referenced this pull request Aug 27, 2024
maiieul added a commit to maiieul/qwik that referenced this pull request Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants