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

adjusting cloudflare adapter and solid ssr to work together #4888

Merged
merged 10 commits into from
Oct 6, 2022

Conversation

AirBorne04
Copy link
Contributor

Changes

  • cloudflare adapter does now respect user config for ssr
  • solid integration is now better configured for ssr so that it works with the adapter adjustments

Testing

Docs

  • adjusting some settings to make the integration work together as would be expected

@changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2022

🦋 Changeset detected

Latest commit: f161c5d

The changes in this PR will be included in the next version bump.

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 github-actions bot added pkg: solid Related to Solid (scope) pkg: integration Related to any renderer integration (scope) labels Sep 27, 2022
this needs to happen for worker build in order to have the correct build mode for the framework, which needs the nodejs no matter if it is for node or the browser.
@@ -0,0 +1,7 @@
---
'@astrojs/cloudflare': major
Copy link
Contributor

Choose a reason for hiding this comment

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

If this only fixes bugs then this shouldn't be a major. Maybe you were just being conservation here, or can you tell me what this breaks?

If this is a true breaking change I think we should have more explanation of the change in the changeset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adjusted this one to minor

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Still discussing this change so let's not merge yet. I think there's a solution here that will work, but one thing we probably need to do (especially if this is a major) is expose esbuild options as config.

@AirBorne04
Copy link
Contributor Author

I think there's a solution here that will work, but one thing we probably need to do (especially if this is a major) is expose esbuild options as config.

There is not really a need to expose the esbuild options as config i think, unless there is lib that needs to import the node config from a package (and you are running with react) but you could also precompile than manually.

the logic this adapter now follows is

  • build the framework for the ssr target (solid uses node, react uses browser)
  • bundle the rest of the libs used with esbuild for the platform browser

especially if this is a major
from my point of view it does not need to be a major

  • the solid integration it is just fixing something that was anyway broken
  • the cloudflare adapter is actually changing the import, but does fix a bug (but might break something on other integrations like vue, preact ...)

Is there a minimal repo for the other frameworks (svelte, preact, react, vue, lit) to check againts. I am willing to run through them and check if they generally work with this config in ssr mode, or if there is a fix needed for any of them, to get this working more stable?

@matthewp
Copy link
Contributor

@AirBorne04 there is the framework-* in the examples/ folder. Can you test against this?

I understand the trepidation about adding too much config. If you want to check out the other frameworks and let me know, I'd be happy to proceed with the current changes.

The only other thing I would ask for is a lengthier (paragraph or two) explanation of the changes in the changeset.

@AirBorne04
Copy link
Contributor Author

hi,

i finally got to do the work. Nearly all example projects are running fine, only lit is not working out of the box. The issue here is that the frameworks ssr package heavily relies on node functionality. This is currently discussed in an issue here #4999 and therefore i think it is out of scope getting it to work, before the core team has enabled this functionality.

@matthewp
Copy link
Contributor

matthewp commented Oct 6, 2022

Thanks @AirBorne04, I agree, the issue here wasn't to try and get every framework to work with Cloudflare. Just making sure this solution didn't break the ones that people were already using, and it sounds like you achieved that. Thank you!

@matthewp matthewp merged commit 2dc582a into withastro:main Oct 6, 2022
@astrobot-houston astrobot-houston mentioned this pull request Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: solid Related to Solid (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants