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

Add build.assetsPrefix option for CDN support #6714

Merged
merged 24 commits into from
Apr 5, 2023
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Mar 30, 2023

Changes

RFC: withastro/roadmap#545

The changes are honestly base -> assetsPrefix || base everywhere for asset links. And judiciously handle / where base needs to be prepended by /, and assetsPrefix doesn't.

Testing

Added tests for links, images (experimental and @astrojs/image), content collections, and SSR.

Manual live testing: https://github.com/bluwy/astro-cdn-test and https://bluwy.github.io/astro-cdn-test/

Docs

Document import.meta.env.ASSETS_PREFIX at withastro/docs#2971.

Documentation for build.assetsPrefix is in this PR's @astro/types.ts

@changeset-bot
Copy link

changeset-bot bot commented Mar 30, 2023

🦋 Changeset detected

Latest commit: 168e353

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

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) labels Mar 30, 2023
@bluwy bluwy marked this pull request as ready for review March 31, 2023 14:36
@bluwy bluwy requested a review from a team as a code owner March 31, 2023 14:36
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Mar 31, 2023
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Looking good! Left some tiny edits for consideration! 🙌

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Looks like our minor GH action isn't work.

Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Docs LGTM!

@bluwy
Copy link
Member Author

bluwy commented Apr 3, 2023

[DOCS] I've pushed another commit to explain the process of serving assets through a third-party domain as suggested by @delucis (7af5d2a)

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I think this looks good to me. What's the expectation of the value from the point of view of the user? It's marked as a string, but if the user puts "anything_fancy", what's the outcome of the build? Should we do some validation? Should we warn the user?

@bluwy
Copy link
Member Author

bluwy commented Apr 3, 2023

It would act as a normal prefix so it could be anything. I'm mostly testing values like https://cdn.example.com for now, but it's intentionally lax to allow values like /some/other/root if their deployment setup relies on it.

@ematipico
Copy link
Member

Perfect! Thank you for clarifying!

@matthewp
Copy link
Contributor

matthewp commented Apr 3, 2023

@bluwy I think in config.ts we specify site to be a URL (it gets refined with new URL()` so maybe it makes sense to do the same here, so that any non-URLs would throw?

@bluwy
Copy link
Member Author

bluwy commented Apr 4, 2023

The idea is that it doesn't necessarily need to be a URL, it can be any string prefix depending on their deployment, so I'm not sure if we need that restriction, unless we want to only support URL for now?

@matthewp
Copy link
Contributor

matthewp commented Apr 4, 2023

Ah, no, good point. It could just be a path. In that case I agree, let's leave it as a string for now.

@bluwy bluwy merged commit ff04307 into main Apr 5, 2023
@bluwy bluwy deleted the plt-44-implement-pr branch April 5, 2023 13:31
@astrobot-houston astrobot-houston mentioned this pull request Apr 5, 2023
@gkatsanos
Copy link

It would act as a normal prefix so it could be anything. I'm mostly testing values like https://cdn.example.com for now, but it's intentionally lax to allow values like /some/other/root if their deployment setup relies on it.

I think I found an issue with this second type of values: #6844

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants