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

Use Workerd for local development #1184

Merged
merged 44 commits into from Sep 20, 2023
Merged

Use Workerd for local development #1184

merged 44 commits into from Sep 20, 2023

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Aug 3, 2023

This removes the @shopifiy/mini-oxygen dependency, which relies on Miniflare v2, and directly relies on Miniflare v3 + Workerd for local development.
Add --worker-unstable flag to dev and preview commands to run the app on Workerd (Miniflare v3).
Without this flag, it runs in a Node.js sandbox (MiniOxygen/Miniflare v2).

Part of the logging logic is taken from Wrangler: it establishes a WS connection to DevTools and listens for events to log messages and errors.

🎩

  • Play with skeleton or demo-store and see if there's anything wrong.

@github-actions

This comment has been minimized.

@frandiox frandiox requested a review from a team August 3, 2023 18:28
Copy link
Contributor

@vincentezw vincentezw left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this, amazing work!

Using the Chrome dev tools protocol to interact with workerd is very clever and not something I had looked into myself. It does lead to quite an amount of complexity, and I'm finding the code a little bit hard to read. I can see the value it adds over miniflare's own logging, but did experience some volatility at times with Miniflare crashing with non-descript errors (nothing I can replicate though).
Do you think this is a viable route? Or should we aim to modify miniflare to allow for these use-cases without having to mimic Wrangler's methods?

packages/cli/src/lib/mini-oxygen.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/mini-oxygen.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/mini-oxygen.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/mini-oxygen.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/mini-oxygen.ts Outdated Show resolved Hide resolved
@frandiox
Copy link
Contributor Author

@vincentezw Thanks for the review!

I can see the value it adds over miniflare's own logging, but did experience some volatility at times with Miniflare crashing with non-descript errors (nothing I can replicate though).

Miniflare/workerd's logging is kind of awkward and not complete, so we can't rely on that. For example, it does not show stack traces from errors. Also, every message is colored and mixed with other cryptic information about C++. It only shows stuff when using verbose: true, and you get a lot of other things that needs to be carefully filtered.
Therefore, until logging is improved in Workerd, I think the only solution is relying on dev tools protocol like Wrangler does :/ -- This also would allows us to connect from Chrome dev tools (not possible in this PR but can be implemented) to show profiler and other things.

I've seen some unexpected crashing from Miniflare as well but I think it wasn't related to dev tools. Something to debug deeper before releasing for sure.

Do you think this is a viable route? Or should we aim to modify miniflare to allow for these use-cases without having to mimic Wrangler's methods?

As mentioned above, I don't think we can do anything else with logs apart from using dev tools, unless we want to jump into workerd direclty (and even like that, it would probably take a long time to be reviewed and released).

Wrangler uses this method by default when running workers locally so it's probably good enough.
That said, if we do upgrade to using Workerd, we should make an unstable release and ask a few developers to test their apps before we make this the default path. We could also release this under an unstable flag in the CLI... but I'm afraid that installation times will double considering the size of Miniflare 2 + Miniflare 3.

@juanpprieto
Copy link
Contributor

juanpprieto commented Aug 14, 2023

Looking good! All skeleton routes seem to be working ok.

The only thing I noticed is a few instances of this log (maybe is my connection?):

   GET  200  loader  /products/half-zip  21ms  [root]  prefetch
A promise rejection was handled asynchronously. This warning occurs when attaching a catch handler to a promise after it rejected. (rejection #1)
   GET  200  loader  /products/half-zip  246ms  [products.$handle]  prefetch
[Error: Network connection lost.] { [cause]: undefined }
  POST  200  action  /cart  631ms  [cart]
   GET  200  loader  /products/half-zip  22ms  [root]
   GET  200  loader  /products/half-zip  12ms  [products.$handle]
   GET  200  loader  /products/half-zip  17ms  [root]
   GET  200  loader  /products/half-zip  9ms  [products.$handle]
   GET  200  loader  /products/half-zip  21ms  [root]  prefetch
   GET  200  loader  /products/half-zip  238ms  [products.$handle]  prefetch
  POST  200  action  /cart  553ms  [cart]
   GET  200  loader  /products/half-zip  23ms  [products.$handle]
   GET  200  loader  /products/half-zip  16ms  [root]
   GET  200  loader  /products/half-zip  21ms  [root]
A promise rejection was handled asynchronously. This warning occurs when attaching a catch handler to a promise after it rejected. (rejection #2)
[Error: Network connection lost.] { [cause]: undefined }
   GET  200  loader  /products/half-zip  21ms  [root]
   GET  200  loader  /products/half-zip  21ms  [products.$handle]
  POST  200  action  /api/predictive-search  2ms  [api.predictive-search]
   GET  200  loader  /products/half-zip  13ms  [root]
   GET  200  loader  /products/half-zip  14ms  [products.$handle]
  POST  200  action  /api/predictive-search  356ms  [api.predictive-search]
[Error: Network connection lost.] { [cause]: undefined }
  POST  200  action  /api/predictive-search  355ms  [api.predictive-search]
   GET  200  loader  /products/half-zip  20ms  [root]
   GET  200  loader  /products/half-zip  14ms  [products.$handle]
   GET  200  loader  /collections/shoes  28ms  [root]
   GET  200  loader  /collections/shoes  322ms  [collections.$handle]
   GET  200  render  /collections/shoes?_pos=1&_psq=as&_ss=e&_v=1.0  379ms

@@ -35,7 +36,7 @@ export async function runPreview({

const miniOxygen = await startMiniOxygen({
root,
port,
port: await findPort(port),
Copy link
Contributor

Choose a reason for hiding this comment

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

🤟🏼

@vincentezw
Copy link
Contributor

vincentezw commented Aug 15, 2023

Wrangler uses this method by default when running workers locally so it's probably good enough. That said, if we do upgrade to using Workerd, we should make an unstable release and ask a few developers to test their apps before we make this the default path. We could also release this under an unstable flag in the CLI... but I'm afraid that installation times will double considering the size of Miniflare 2 + Miniflare 3.

I think an unstable release sounds very reasonable. fwiw the difference in size between miniflare2-3 is rather small (280k vs 1.8m) but mini-oxygen is quite bloated as a dependency).

@frandiox
Copy link
Contributor Author

fwiw the difference in size between miniflare2-3 is rather small (280k vs 1.8m) but mini-oxygen is quite bloated as a dependency).

I'm assuming you're using Bundlephobia to check the sizes but I think that service is not super accurate. The workerd binary alone is 60+ MB on Mac and ~75 MB on Linux I think.

image

Using a more accurate tool:

Therefore, maybe we could install miniflare@3 in the project lazily when --workerd flag is used...
Perhaps there's a case for maintaining both mini-oxygen@2 and mini-oxygen@3 and make them part of the user's package.json, then we use whatever is installed 🤔

@github-actions github-actions bot had a problem deploying to preview August 16, 2023 00:44 Failure
@blittle
Copy link
Contributor

blittle commented Aug 16, 2023

@juanpprieto I think the connection lost and promise rejection errors are due to the connection getting cancelled from the browser. It happens easily when navigating quickly between pages, so before a loader request can finish, a new one starts so the previous is cancelled. Or a prefetch request that doesn't finish before navigating, so it gets cancelled
image

I guess the question is, should these be showing up in the logs. Are devs going to get confused and freak out?

@github-actions github-actions bot had a problem deploying to preview August 17, 2023 04:36 Failure
@frandiox frandiox changed the base branch from 2023-07 to main August 28, 2023 11:27
@frandiox
Copy link
Contributor Author

Waiting for the next release of Miniflare which drops a dependency that can make installation times considerably slower.

@frandiox frandiox merged commit e62a4db into main Sep 20, 2023
10 checks passed
@frandiox frandiox deleted the fd-mini-oxygen-workerd branch September 20, 2023 07:30
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.

None yet

5 participants