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

fix(cloudflare): support for 'cloudflare:*' imports #8766

Merged
merged 7 commits into from
Oct 9, 2023

Conversation

jadbox
Copy link
Contributor

@jadbox jadbox commented Oct 6, 2023

Changes

  • Allow the use of the runtime library cloudflare:sockets without throwing a build error by adding it to the external list.

WHY?

This update is needed for merge in order to further work on Postgres.js to allow for SQL socket connections for the CF tcp connec() api.

Testing

Try to reference cloudflare:sockets in an import.

Docs

https://blog.cloudflare.com/workers-tcp-socket-api-connect-databases/

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2023

🦋 Changeset detected

Latest commit: 35f8a29

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 the pkg: integration Related to any renderer integration (scope) label Oct 6, 2023
Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

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

Clean implementation, thanks!

You should know the cloudflare adapter is in the middle of a migration to https://github.com/withastro/adapters. Hopefully, it will be done by the end of next week. This PR would probably be merged in that repo.

Copy link
Member

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, do you think it would be better to support all cloudflare: prefixed imports, given that there might be more right now and coming in the future?

Also I'm just going to block this for now, since as @lilnasy said, we would probably merge it in the new repo. I'll follow up at least every other day to keep you in the loop!

packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
@jadbox
Copy link
Contributor Author

jadbox commented Oct 8, 2023

@alexanderniebuhr thanks for the update. This is a project blocker for us until resolved, but I understand the need to finish the adapter repo change.

If we can use wildcard for cloudflare:* packages (and that's esbuild valid syntax?), I think we should also wildcard all node:* packages too since CF continually adds new packages here.

On a large point outside of esbuild externals, it seems limiting presently to not be able to override any of the esbuild settings in the client from outside the adapter. Cloudflare updates frequently, and the adapter seems to need an update every time due to esbuild directives. The above pkg external wildcards would help, but not in all cases. Maybe the adapter(s) client interface should take an optional esbuild parameter selective override config object?

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Oct 8, 2023

This is a project blocker for us until resolved.

Ok, I understand. We can also get this PR finished here, but would need to open the same PR on the new repo too.
I would like to see added documentation, for the feature (depending if we go for the wildcard or just the sockets) & link to the related Cloudflare docs for cloudflare: imports.

If we can use wildcard for cloudflare:* packages (and that's esbuild valid syntax?)

I think it is valid syntax, would you mind to research it? IIRC the SvelteKit Cloudflare adapter uses cloudflare:*.

I think we should also wildcard all node:* packages too since CF continually adds new packages here.

I don't think this is a valid approach. What happens if the user uses a Node.JS Api which is not supported? We would not warn/stop the user, but running the project would fail, not great dx. The difference here is that all possible imports from cloudflare:*, will be supported by Cloudflare. While for the node:* prefix only a subset of imports are supported, so we should be very explicit about the supported node options and should only externalize those.

On a large point outside of esbuild externals, it seems limiting presently to not be able to override any of the esbuild settings in the client from outside the adapter. Cloudflare updates frequently, and the adapter seems to need an update every time due to esbuild directives. The above pkg external wildcards would help, but not in all cases. Maybe the adapter(s) client interface should take an optional esbuild parameter selective override config object?

Thank you for the idea. While I personally don't like to allow esbuild settings overwrites, it's definitely outside the current PR's scope. Let's explore this in a separate PR to fully address its implications and documentation needs. I personally would like to see a broader discussion first, regarding this idea. IIRC the Svelte team already came up with some arguments in favor & against that idea. (I think you even linked them yourself, somewhere here withastro/adapters#22)

@jadbox
Copy link
Contributor Author

jadbox commented Oct 8, 2023

@alexanderniebuhr I've committed a patch to wildcard cloudflare:* packages.

@alexanderniebuhr
Copy link
Member

Thank you. Can you also add / update the documentation in the README.md file?

@jadbox jadbox requested a review from a team as a code owner October 9, 2023 14:39
@jadbox
Copy link
Contributor Author

jadbox commented Oct 9, 2023

@alexanderniebuhr Readme updated under ## Cloudflare module support

@alexanderniebuhr
Copy link
Member

@jadbox thank you so much. Once docs approve, we will get this merged here & I will make sure it get's moved over to the new repo!

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thanks for adding these docs @jadbox ! Just a few tiny changes to language/formatting to get it in to docs. Then, Docs is happy! 🙌

.changeset/heavy-elephants-tan.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
@jadbox
Copy link
Contributor Author

jadbox commented Oct 9, 2023

@sarah11918 Requested doc changes committed.

@alexanderniebuhr alexanderniebuhr changed the title chore: add 'cloudflare:sockets' to external list fix(cloudflare): support for 'cloudflare:*' imports Oct 9, 2023
@alexanderniebuhr alexanderniebuhr merged commit 054c5c6 into withastro:main Oct 9, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 9, 2023
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants