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(templates): update Cloudflare templates to use wrangler v2 #3160

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

aiji42
Copy link
Contributor

@aiji42 aiji42 commented May 11, 2022

Update template for cloudflare workers. (#3158)

  • wrangler.toml: Changed to wrangler v2 style.
  • package.json: Added wrangler to dependecncies. Updated @cloudflare/workers-types latest. (also pages template)
  • remix.env.d.ts: Fixed dead link. (also pages template)
  • README: Removed the sentence about installation of wrangler.

I have confirmed that it is deployable. (https://github.com/aiji42/remix-cf-wrangler-2)

uejima.aiji$ yarn deploy                                                                                             [~/git/remix-cf-wrangler-2]
yarn run v1.22.18
$ npm run build && wrangler publish

> build
> remix build

Building Remix app in production mode...
Built in 385ms
 ⛅️ wrangler 2.0.2
-------------------
Running custom build: npm run build

> build
> remix build

Building Remix app in production mode...
Built in 368ms
Reading public/build/_shared/chunk-LHODYXPX.js...
Skipping - already uploaded.
Reading public/build/_shared/chunk-OGYP3M3B.js...
Skipping - already uploaded.
Reading public/build/entry.client-GY23DEVV.js...
Skipping - already uploaded.
Reading public/build/manifest-289EFF28.js...
Skipping - already uploaded.
Reading public/build/root-NVPFOTJV.js...
Skipping - already uploaded.
Reading public/build/routes/index-UD6EN4U7.js...
Skipping - already uploaded.
Reading public/favicon.ico...
Skipping - already uploaded.
↗️  Done syncing assets
Uploaded remix-cf-wrangler-2 (2.18 sec)
Published remix-cf-wrangler-2 (0.31 sec)
  remix-cf-wrangler-2.aiji422990.workers.dev
✨  Done in 7.46s.

templates/cloudflare-pages/package.json Outdated Show resolved Hide resolved
templates/cloudflare-workers/wrangler.toml Outdated Show resolved Hide resolved
@MichaelDeBoey MichaelDeBoey changed the title fix(templates): update cloudflare templates (use wrangler v2) feat(templates): update Cloudflare templates to use wrangler v2 May 11, 2022
@MichaelDeBoey
Copy link
Member

@jacob-ebey Is there a reason why cloudflare-pages template doesn't have a wrangler.toml file? 🤔
It seems odd that we have one in cloudflare-workers but not in cloudflare-pages, even though we have wranglerindevDependencies`

@himorishige
Copy link
Contributor

Thank you for providing a useful template.

One suggestion would be.

The kv-asset-handler installed from the template seemed to be v0.1.3.
Since v0.2.0, some definitions have changed, such as getAssetFromKV, etc.
I would like to see kv-asset-handler also bundled with v0.2.0 or higher.

https://github.com/cloudflare/kv-asset-handler/releases/tag/v0.2.0

@MichaelDeBoey
Copy link
Member

@aiji42 Could you please rebase this PR onto latest main & resolve conflicts?

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 17, 2022
@aiji42
Copy link
Contributor Author

aiji42 commented May 17, 2022

@MichaelDeBoey I have resolved the conflicts! 👍

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 17, 2022
@aiji42
Copy link
Contributor Author

aiji42 commented May 17, 2022

@himorishige
I am not familiar with @cloudflare/kv-asset-handler, please give me more details. If you don't update it, will you run into trouble?
This PR is some changes to templates and will not involve a release since the merge target is the main branch. If the update is mandatory, then this PR is not sufficient. However, if it is not mandatory, then it should be fixed in another (release-oriented) one.

@acusti
Copy link
Contributor

acusti commented May 17, 2022

@aiji42 thanks for putting this together! i just applied your changes manually (updating wrangler.toml and adding wrangler v2.0.5 as a dev dependency) and it works perfectly.


for anyone else running into issues and searching for answers, i originally tried to create a new remix project using cloudflare-workers as the build target, and when i installed wrangler using the instructions in the doc that the remix template’s README links to and ran npm run deploy, i got the following output:

 ⛅️ wrangler 2.0.5
-------------------
▲ [WARNING] Processing wrangler.toml configuration:

    - 😶 Ignored: "type":
      Most common features now work out of the box with wrangler, including modules, jsx,
  typescript, etc. If you need anything more, use a custom build.
    - Deprecation: "zone_id":
      This is unnecessary since we can deduce this from routes directly.
    - Deprecation: "build.upload.format":
      The format is inferred automatically from the code.
    - Deprecation: "site.entry-point":
      Delete the `site.entry-point` field, then add the top level `main` field to your configuration
  file:
      ```
      main = "index.js"
      ```


✘ [ERROR] Processing wrangler.toml configuration:

    - Expected "route" to be either a string, or an object with shape { pattern, zone_id | zone_name
  }, but got "".

replacing the contents of wrangler.toml with this will make it work:

name = "remix-cloudflare-workers"

compatibility_date = "2022-05-11"

account_id = ""
workers_dev = true
main = "./build/index.js"

[site]
bucket = "./public"

[build]
command = "npm run build"

@himorishige
Copy link
Contributor

@aiji42

Thanks for your feedback. As you indicated, it may not necessarily need to be updated as a template. We will withdraw this proposal.
@cloudflare/kv-asset-handler is used to create a Worker environment that is an adapter instead of server.js when using DO. But I don't think that is necessary for the template.

@zwily
Copy link

zwily commented May 19, 2022

Wrangler 2 also includes miniflare, so should this change also remove the explicit miniflare dependency from cloudflare-workers and change the start and dev:miniflare scripts to call wrangler dev --local?

@zwily
Copy link

zwily commented May 20, 2022

Wrangler 2 also includes miniflare, so should this change also remove the explicit miniflare dependency from cloudflare-workers and change the start and dev:miniflare scripts to call wrangler dev --local?

Except, I just found this in the Cloudflare Discord:

  • Workers Sites projects will still deploy with wrangler2, however, you can no longer create them. We recommend using Cloudflare Pages instead.

Given that Remix on Workers relies on being a worker site, shouldn't we stick with wrangler 1?

@zwily
Copy link

zwily commented May 26, 2022

  • Workers Sites projects will still deploy with wrangler2, however, you can no longer create them. We recommend using Cloudflare Pages instead.

Given that Remix on Workers relies on being a worker site, shouldn't we stick with wrangler 1?

I just tested this - I created a new Workers app with npx create-remix, and then applied the changes in this patchset to it to get it on wrangler 2, and it created and deployed the site just fine. So I'm not sure what that note in the Cloudflare Discord could be referring to.

@terribleplan
Copy link

terribleplan commented May 31, 2022

I arrived at these changes independently of seeing this PR since the CF docs now say to use wrangler2 from NPM. Most things seem to be working except for static asset serving.

I can see the assets get uploaded, stored in a KV namespace, and be requested by a client, but something either in @cloudflare/kv-asset-handler or how it is called is not working correctly, and I am getting a 404 for files under /build.

I tried messing with publicPath in remix.config.js (as the assets in KV are prefixed with public/build and I am unsure how mapping from a request path to KV key works) with no change.

I wonder if this might be related to the "you can no longer create them" mentioned above, as this is a new worker/site, and as far as I can tell everything should be working. Using pages is also sadly a non-starter at the moment as pages does not support R2 bindings yet and the (CF) docs poorly explain the new manual upload deployment method (as this project isn't in github/gitlab).

ETA: It looks like serving favicon works, so it may be some deeper issue.

It is also worth noting that I had to manually resolve miniflare (when using yarn at least) as different versions were being installed to satisfy the direct dev dependency vs the dependency through wrangler, which was causing weird issues in dev (failing to parse incoming requests).

  "resolutions": {
    "miniflare": "^2.5.0"
  }

@pyrossh
Copy link

pyrossh commented Jun 9, 2022

I got it working with the above instructions on wrangler 2. The images are working fine for me as I import them in code though not served statically.

@n3oney
Copy link

n3oney commented Jun 28, 2022

Just a question. Does the deploy script in the package.json actually need the npm run build before wrangler publish, if we already have a npm run build in the build section of the wrangler.toml? It seems like it's going to build it twice.

@john-griffin
Copy link

What is left to do here? Need a hand with anything? Wrangler 2 is the official way forward and I'm seeing a lot of people in the community confused when generating a new app.

@MichaelDeBoey
Copy link
Member

@aiji42 Could you please rebase your branch on latest dev & resolve conflicts?

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Jul 24, 2022
@aiji42
Copy link
Contributor Author

aiji42 commented Jul 24, 2022

@MichaelDeBoey
Is it latest dev, not latest main? I have main as the target branch since it is a template change.
Also, the version of wrangler has been upgraded, so I will update to use the latest.

@changeset-bot
Copy link

changeset-bot bot commented Jul 24, 2022

⚠️ No Changeset found

Latest commit: 9be7d88

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -1,6 +1,5 @@
name = "remix-cloudflare-workers"

account_id = ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed account_id due to the following warning during deployment.

$ wrangler publish
 ⛅️ wrangler 2.0.22
--------------------
▲ [WARNING] Processing wrangler.toml configuration:

    - The "account_id" field in your configuration is an empty string and will be ignored.
      Please remove the "account_id" field from your configuration.

@aiji42
Copy link
Contributor Author

aiji42 commented Jul 24, 2022

I rebased to the latest main and then resolved the conflicts. I updated to the latest wrangler.

Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Still wondering if we also need a wrangler.toml for cloudflare-pages 🤔

CC/ @jacob-ebey

@MichaelDeBoey MichaelDeBoey removed the needs-response We need a response from the original author about this issue/PR label Jul 24, 2022
@aiji42
Copy link
Contributor Author

aiji42 commented Jul 24, 2022

@MichaelDeBoey
No need for a wrangler.toml in cloudflare-pages application.
This is because the repositories are linked from the Cloudflare Pages dashboard to build and deploy.
https://developers.cloudflare.com/pages/framework-guides/remix/

Copy link
Member

@kentcdodds kentcdodds 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. Thank you!

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.

None yet