Skip to content

Conversation

@jculvey
Copy link
Contributor

@jculvey jculvey commented Nov 29, 2023

Fixes #4514.

What this PR solves / how to test:

Workers projects created via c3 get a stale version of @cloudflare/workers-types from their template files and use the default entrypoint in their tsconfig.json. This makes for a rough experience for typescript users.

This change fixes that by installing both wrangler and @cloudflare/workers-types on demand, and by looking up the latest version of the types entrypoint and adjusting the tsconfig.

Author has addressed the following:

  • Tests
    • [ x ] Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • [ x ] Included
    • Not necessary because:
  • Associated docs
    • Issue(s)/PR(s):
    • Not necessary because:

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

Installing these as part of the generator instead of relying on
a stale version in the template we copied ensures we have the most
up-to-date version.
@jculvey jculvey requested a review from a team November 29, 2023 21:44
@changeset-bot
Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: 2e08b32

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 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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 29, 2023

A create-cloudflare (C3) prerelease is available for testing.
You can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs//npm-package-create-cloudflare-

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

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Looks good to me 🙂

The tests look really nice! 🤩👍

As I mentioned in chat, I am not really convinced that installing wrangler and @cloudflare/workers-types is the right approach as having them in the package.jsons with ^ should be enough, but if you strongly believe that it is I am fine with it 🙂

@jculvey
Copy link
Contributor Author

jculvey commented Nov 30, 2023

As I mentioned in chat, I am not really convinced that installing wrangler and @cloudflare/workers-types is the right approach as having them in the package.jsons with ^ should be enough, but if you strongly believe that it is I am fine with it 🙂

I tell you what, let's compromise. I'll leave wrangler in the templates and keep @cloudflare/workers-types where it is since it's highly version sensitive. I worry that without this change users of pnpm between 8.0.0 and 8.7.0 will have a bad experience when they get old types (from April) due to the resolution-mode issue (see more here).

FWIW this code will likely be revisited in the near future as we add support for non built-in templates.

@dario-piotrowicz
Copy link
Member

dario-piotrowicz commented Nov 30, 2023

@jculvey no please don't worry I am totally fine with installing both wrangler and @cloudflare/workers-types I just kind of left the comment to be fully transparent not to get you to change things 🙂, please feel free to do as you prefer I also don't think that installing things on the fly is likely to cause issues (it just feels a bit of an unnecessary extra step to me)

@codecov
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #4525 (2e08b32) into main (18a4dd9) will increase coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4525      +/-   ##
==========================================
+ Coverage   75.44%   75.48%   +0.03%     
==========================================
  Files         240      240              
  Lines       12851    12851              
  Branches     3309     3309              
==========================================
+ Hits         9695     9700       +5     
+ Misses       3156     3151       -5     

see 6 files with indirect coverage changes

@jculvey jculvey merged commit 294ca54 into main Nov 30, 2023
@jculvey jculvey deleted the c3-bump-workers-types branch November 30, 2023 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

C3: Improve @cloudflare/workers-types types used for Workers

2 participants