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

make sure C3 works with the latest astro versions #7497

Merged
merged 1 commit into from
Dec 16, 2024
Merged

make sure C3 works with the latest astro versions #7497

merged 1 commit into from
Dec 16, 2024

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Dec 10, 2024 •
edited
Loading

fixes #7416

This PR makes sure that both the stable and experimental C3 Astro templates work with the latest Astro versions (which are currently broken since they no longer include an src/env.d.ts file in their starting templates)

  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: C3 e2e tests for Astro are already present
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: there are no functional changes here

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 10, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 6488dc4

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

This PR includes changesets to release 1 package
Name Type
create-cloudflare Patch

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
Copy link
Contributor

github-actions bot commented Dec 10, 2024 •
edited
Loading

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-wrangler-7497

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7497/npm-package-wrangler-7497

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-wrangler-7497 dev path/to/script.js
Additional artifacts: 
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-create-cloudflare-7497 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-cloudflare-kv-asset-handler-7497
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-miniflare-7497
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-cloudflare-pages-shared-7497
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-cloudflare-vitest-pool-workers-7497
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-cloudflare-workers-editor-shared-7497
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-cloudflare-workers-shared-7497
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-cloudflare-workflows-shared-7497

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.94.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241205.0
workerd 1.20241205.0 1.20241205.0
workerd --version 1.20241205.0 2024-12-05

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@dario-piotrowicz dario-piotrowicz added the e2e Run e2e tests on a PR label Dec 10, 2024
dario-piotrowicz
dario-piotrowicz commented Dec 10, 2024
View reviewed changes
packages/create-cloudflare/templates-experimental/astro/c3.ts
Comment on lines +68 to +74
async selectVariant(ctx) {
// Note: this `selectVariant` function should not be needed
// this is just a quick workaround until
// https://github.com/cloudflare/workers-sdk/issues/7495
// is resolved
return usesTypescript(ctx) ? "ts" : "js";
},
Copy link
Member Author

@dario-piotrowicz dario-piotrowicz Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Funnily enough, it looks like this isn't really needed since it looks like the Astro CLI creates a tsconfig.json for you even if you tell it that you're not planning on using TypeScript 🤷

I would still keep this in as I think this is the proper implementation (workaround excluded) and it is more robust (it won't break if the Astro CLI changes the above behavior and stops creating the tsconfig file)

@dario-piotrowicz
fix: make sure C3 works with the latest Astro versions
b8a15f4 
make sure that both the stable and experimental C3 Astro templates
work with the latest Astro versions (which no longer include
an src/env.d.ts file in their starting templates)
@dario-piotrowicz dario-piotrowicz added the c3 Relating to C3 (create-cloudflare) package label Dec 10, 2024
@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review December 10, 2024 12:06
@dario-piotrowicz dario-piotrowicz requested review from a team as code owners December 10, 2024 12:06
@alexanderniebuhr
Copy link
Contributor

alexanderniebuhr commented Dec 10, 2024 •
edited
Loading

So adding the src/env.d.ts yourself works as a quick hotfix, but I would recommend to implement a different solution in the long term, but we were waiting for Cloudflare to implement a types API.. See this POC for context withastro/adapters#355 (comment)

This was referenced Dec 10, 2024
Closed
Closed
Closed
@dario-piotrowicz
Copy link
Member Author

but we were waiting for Cloudflare to implement a types API..

Yes, we've discussed this already, so sorry that we still haven't provided the API for you 🙇

@andyjessop are there any updates / potential planned work on the API?

@andyjessop
Copy link
Contributor

but we were waiting for Cloudflare to implement a types API..

Yes, we've discussed this already, so sorry that we still haven't provided the API for you 🙇

@andyjessop are there any updates / potential planned work on the API?

Hey @dario-piotrowicz! Thanks for the ping. I've spoken about this internally and I think we can start work on it straightaway.

@daveio
Copy link

daveio commented Dec 13, 2024

Sorry to comment without much to add, but is there any way of invoking C3 with this changeset? I'm blocked on creating an Astro app.

@petebacondarwin
Copy link
Contributor

I think you want:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12255380940/npm-package-create-cloudflare-7497 --no-auto-update

@CarmenPopoviciu CarmenPopoviciu merged commit fb30983 into main Dec 16, 2024
40 of 42 checks passed
@CarmenPopoviciu CarmenPopoviciu deleted the dario/7492/astro-c3-fix branch December 16, 2024 17:10
@workers-devprod workers-devprod mentioned this pull request Dec 16, 2024
Merged
penalosa pushed a commit that referenced this pull request Jan 10, 2025
@dario-piotrowicz @penalosa
fix: make sure C3 works with the latest Astro versions (#7497)
2f5932e 
make sure that both the stable and experimental C3 Astro templates
work with the latest Astro versions (which no longer include
an src/env.d.ts file in their starting templates)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@penalosa penalosa penalosa approved these changes

Assignees
No one assigned
Labels
c3 Relating to C3 (create-cloudflare) package e2e Run e2e tests on a PR
Projects
workers-sdk
Status: Done
Milestone
No milestone
7 participants
@dario-piotrowicz @alexanderniebuhr @andyjessop @daveio @petebacondarwin @penalosa @CarmenPopoviciu