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

[Miniflare 3] Add native support for Windows 🎉 #551

Merged
merged 5 commits into from
Apr 17, 2023
Merged

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Apr 5, 2023

As of cloudflare/workerd#452, workerd supports Windows natively. This change upgrades workerd, then removes the WSL and Docker runtime shells. It also Cherrypicks #509, to allow Miniflare development on Windows too.

@mrbbot mrbbot requested review from jspspike and penalosa April 5, 2023 13:45
@changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2023

⚠️ No Changeset found

Latest commit: 365cb10

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.

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

@mrbbot mrbbot changed the title Add native support for Windows 🎉 [Miniflare 3] Add native support for Windows 🎉 Apr 5, 2023
@mrbbot mrbbot force-pushed the tre-native-windows branch 3 times, most recently from cccd3da to a6bc822 Compare April 5, 2023 14:26
@mrbbot mrbbot marked this pull request as draft April 6, 2023 13:53
@mrbbot
Copy link
Contributor Author

mrbbot commented Apr 6, 2023

Switching to draft while I figure out what's going wrong with CI... :(

@mrbbot mrbbot force-pushed the tre-native-windows branch 9 times, most recently from dca740b to 980448c Compare April 12, 2023 16:27
@mrbbot
Copy link
Contributor Author

mrbbot commented Apr 12, 2023

I think CI should be fixed by fb48926 🤞 See commit descriptions for more details. 🙂

@mrbbot mrbbot marked this pull request as ready for review April 12, 2023 16:31
.github/workflows/test.yml Outdated Show resolved Hide resolved
packages/tre/package.json Show resolved Hide resolved
"lint:fix": "npm run lint -- --fix",
"prepublishOnly": "npm run lint && npm run clean && npm run build && npm run types:bundle && npm run test",
"test": "npm run build && ava && rimraf ./.tmp",
"types:build": "tsc",
"___$comment:types:bundle": "api-extractor doesn't know to load index.ts instead of index.d.ts when resolving imported types",
"types:bundle": "cp node_modules/@cloudflare/workers-types/experimental/index.ts node_modules/@cloudflare/workers-types/experimental/index.d.ts && npm run types:build && node scripts/types.mjs"
"types:bundle": "node scripts/copy.mjs node_modules/@cloudflare/workers-types/experimental/index.ts node_modules/@cloudflare/workers-types/experimental/index.d.ts && npm run types:build && node scripts/types.mjs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed? (As an aside, why copy the exportable types into the ambient types?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows doesn't provide cp, and we'd like to support Miniflare development on Windows too. The problem here is that api-extractor doesn't know to load index.ts instead of index.d.ts when importing types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay—is there on issue for that on the api-extractor repo that could be linked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might actually be a fundamental restriction of API extractor, it only processes .d.ts files. From https://api-extractor.com/pages/setup/invoking/:

This enables generation of the .d.ts files that API Extractor will analyze. By design, TypeScript source files are not directly analyzed, but instead must be first processed by your compiler.

It just so happens in our case, that our .ts files are effectively .d.ts files with additional export statements.

packages/tre/src/index.ts Show resolved Hide resolved
packages/tre/src/wait.ts Show resolved Hide resolved
packages/tre/src/index.ts Show resolved Hide resolved
@penalosa
Copy link
Contributor

This is really exciting! Mostly just a couple questions

mrbbot added 3 commits April 17, 2023 14:04
As of cloudflare/workerd#452, `workerd` supports Windows' natively.
This change upgrades `workerd`, then removes the WSL and Docker
runtime shells. It also `Cherry`picks #509, to allow Miniflare
development on Windows too.
...and disable keep-alives. This was a debugging step, but is
probably a good thing to do anyway. We're making lots of repeated
requests here, `http` is probably more optimised than `undici` atm.
In most cases, the socket will be closed as `workerd` won't be
listening yet, so keep-alives don't make sense.
When `workerd` sends a request to Miniflare's loopback server, we
convert the incoming request to an `undici` `Request` object.
Previously, we passed the incoming request object directly to the
`new Request()` constructor. Internally, `undici` created an async
iterator from this request body stream. Unfortunately, if `workerd`
ever aborted one of these requests (e.g. disposing runtime with a
pending `waitUntil`ed `fetch`), this abort error would be propagated
to the async iterator, and cause an unhandled rejection.

We now convert this incoming request to a `ReadableStream` ourselves,
catching errors from this async iterator, gracefully closing the
stream.
mrbbot added 2 commits April 17, 2023 14:04
Building on the previous commit, avoid sending requests that might be
aborted in tests, because they're `waitUntil`ed. Without this, we
could end up with an incomplete JSON body that can't be parsed.
@mrbbot mrbbot force-pushed the tre-native-windows branch from 8332402 to 365cb10 Compare April 17, 2023 13:05
@mrbbot mrbbot merged commit 32bdee3 into tre Apr 17, 2023
@mrbbot mrbbot deleted the tre-native-windows branch April 17, 2023 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tre Relating to Miniflare 3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants