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

[Bug]: Cloudflare-pages preset not working with Beta 2 #1217

Closed
2 tasks done
dario-piotrowicz opened this issue Jan 4, 2024 · 12 comments
Closed
2 tasks done

[Bug]: Cloudflare-pages preset not working with Beta 2 #1217

dario-piotrowicz opened this issue Jan 4, 2024 · 12 comments
Labels
bug Something isn't working

Comments

@dario-piotrowicz
Copy link

dario-piotrowicz commented Jan 4, 2024

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

It seems that trying to build a solid-start application using vinxi generates a broken non-deployable output

From the terminal logs I think that the fetch handler might be omitted from the build output:
error

Expected behavior 🤔

npm run build should produce a functioning Cloudflare Pages application

Steps to reproduce 🕹

Steps:

  1. Create a new solid start application (docs):
npm init solid@latest
  1. Use the cloudflare-pages preset in your vite.config.ts (docs):
import { defineConfig } from "@solidjs/start/config";

export default defineConfig({
  start: {
    server: {
      preset: "cloudflare-pages"
    }
  }
});
  1. run npm run build, notice in the terminal that the correct preset gets picked up:
    Screenshot 2024-01-04 at 17 34 07

  2. run npx wrangler pages dev dist/ to see the following error in the terminal:
    error

Minimal Reproduction

In case it helps I've created a minimal reproduction for this: https://github.com/dario-piotrowicz/solid-start-beta-2-cloudflare-pages-preset-build-not-working-repro

@edivados
Copy link
Contributor

edivados commented Jan 4, 2024

Let me know if this helps.

Two issues here:

1. Should be the same problem as nksaraf/vinxi#77

Use overrides so you don't get two versions of vinxi. Workaround until new version of solid-start is published.

{
...
 "engines": {
    "node": ">=18"
  },
  "overrides": {
    "vinxi": "0.0.64"
  }
}

2. Preset config

Taken from the start docs source

export default defineConfig({
  start: {
    server: {
      preset: "cloudflare-pages",
      rollupConfig: {
        external: ["__STATIC_CONTENT_MANIFEST", "node:async_hooks"]
      },
    }
  }
});

Not a cloudflare user myself... maybe __STATIC_CONTENT_MANIFEST is not required not sure...

Tested it with:

npx wrangler pages dev dist/ --compatibility-flags="nodejs_compat"

@ryansolid
Copy link
Member

It's because of Async Local Storage. They support it with Node compat flag but unenv won't not patch it by default because that flag isn't default. I know cloudflare_modules works. But unclear if pages works with that setting. Trying to use the default cloudfloare workers with that setting didn't because it complained that it couldn't find a global, because it builds to like a IIFE or something with no modules.

@dario-piotrowicz
Copy link
Author

@edivados Thanks a bunch your suggested solution does the job 😄 👍 ❤️


PS: "__STATIC_CONTENT_MANIFEST" is a workers only thing and unrelated/unneeded in Pages


Use overrides so you don't get two versions of vinxi. Workaround until new version of solid-start is published.

Any ETA on this?
I'm asking since we allow the creation of solidStart applications in our C3 tool which currently generates a broken solidStart application and we want to fix that, if it's going to take long for this to get fixed we will need to manually add the override to generated solidStart apps, otherwise I think we can wait a bit (a few days) for this to get properly fixed upstream

@dario-piotrowicz
Copy link
Author

It's because of Async Local Storage. They support it with Node compat flag but unenv won't not patch it by default because that flag isn't default. I know cloudflare_modules works. But unclear if pages works with that setting. Trying to use the default cloudfloare workers with that setting didn't because it complained that it couldn't find a global, because it builds to like a IIFE or something with no modules.

Using workers for applications (so in nitro the cloudflare and cloudflare_module presets) is actually not really recommended since it uses the deprecated workers sites feature (that besides other things it also involves that ugly __STATIC_CONTENT_MANIFEST import).

So I think that making sure that this works with Pages (the cloudflare_pages preset) is much more valuable than having it working with workers (too bad the cloudflare_pages preset is marked as experimental in the nitro docs since as I said I think that that is the most valuable one 😕)

Thankfully requiring users to add the

rollupConfig: {
  external: ["node:async_hooks"]
},

code doesn't look too too bad to me (especially since we can do that for the in C3)

@ryansolid
Copy link
Member

@dario-piotrowicz Vinxi version should be fixed in @solidjs/start 0.4.3 which I released yesterday.

I'm glad to hear this is all working.

@dario-piotrowicz
Copy link
Author

@ryansolid Thanks a bunch, I just tried it and it works 🙂

I'm also nice to see the config types improved 🤩

There is however a last small issue I think 😅

That's related to the new types (a bit unrelated to this gh issue), is start.solid truly mandatory?
Screenshot 2024-01-06 at 12 04 55

It doesn't really seem to be since the following seems to be working totally fine:
Screenshot 2024-01-06 at 12 05 21

Also notice the lack of the:

rollupConfig: {
  external: ["node:async_hooks"]
},

so... it seems like that's actually no longer needed? 😮

@ryansolid
Copy link
Member

so... it seems like that's actually no longer needed?

I did a partial polyfill. It is imperfect. I suggest keeping that there. Technically if you grab the request at any synchronous entry to your code it works but we need async local storage to support async grabbing of the event. You might not need it in your case, but it may come up again later.

@ryansolid
Copy link
Member

I think we are good now with 0.4.5 fixing the config?

@dario-piotrowicz
Copy link
Author

@ryansolid sorry for the late reply 🙇

Yes I think we're good! 🙂

But before we close this issue I'd like to ask, if setting the rollupConfig is practically necessary for the Cloudflare preset, shouldn't there be some sort of validation to make sure that it is? instead of sort of hoping that the user sets it, and if they don't things might or not work correctly?

@ryansolid
Copy link
Member

Hmm.. I don't usually write custom stuff like that but yeah maybe I can do it in our config. We don't own the presets Nitro does. I could think of looking at the preset setting, although it can also be set via ENV variable. If we check both and see "cloudflare-pages" or "cloudflare-modules" maybe we could automatically add the right stuff. Although is there a way to make it so node compat is always turned on for SolidStart apps?

@dario-piotrowicz
Copy link
Author

Hey @ryansolid, I'm sorry unfortunately currently there isn't any way to always turn on nodejs_compat for SolidStart apps

The team is currently looking into introducing a config file for Pages, once that is supported solid could just set the nodejs_compat flag there and that should get picked up by wrangler during deployment (and dev).

PS: this is an issue I've discussed with the Nitro team as well (cc. @pi0) maybe it'd be worth to sync up and discuss things together?

@ryansolid
Copy link
Member

Ok I've created a new issue to track the config automation and closing this one.

Yeah we all share a common goal here so we should meet up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants