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

feat: Add fastify-cors example #444

Closed

Conversation

akinorimizushima
Copy link

This will add an example using fastify-cors which can receive requests from different hosts or ports.

@CLAassistant
Copy link

CLAassistant commented Mar 10, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@timostamm timostamm 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 the contribution, @akinorimizushima!

We have been discussing how we can make setting up CORS a bit easier. The PR connectrpc/connect-es#526 adds a nice little helper for that. Do you mind waiting for that PR to land so you can use it here?

const server = fastify()

await server.register(cors, {
origin: "*",
Copy link
Member

Choose a reason for hiding this comment

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

This should be origin: true, but with a comment that this is only recommended for development, not for production. Please take a the @fastify-cors documentation.

Comment on lines 25 to 29
allowedHeaders: [
"content-type",
"Connect-Protocol-Version",
"Access-Control-Allow-Origin",
],
Copy link
Member

Choose a reason for hiding this comment

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

We need a couple more headers here, and we also need to set exposedHeaders and methods.

But with connectrpc/connect-es#526, you can simply use the values we export.

@akinorimizushima
Copy link
Author

Hi @timostamm, thanks for heads up. Okay, will work on it once the feature is published.

@akinorimizushima
Copy link
Author

@timostamm Pushed some commits using the upgraded version but #463 is doing the similar thing. So feel free to reject it if this PR is not helpful as an example.

@smaye81
Copy link
Member

smaye81 commented Mar 18, 2023

@timostamm Pushed some commits using the upgraded version but #463 is doing the similar thing. So feel free to reject it if this PR is not helpful as an example.

Hey @akinorimizushima. This is definitely a helpful example. What we are trying to do is make each example (express, fastify, etc.) self-sufficient and showing both a full-stack as well as a CORS example. #463 is an attempt to unify the Express project so that we don't have express, express-cors, fastify, fastify-cors, etc. Once we land #463, we're hoping to have a good pattern to follow and then we'd appreciate it if you could modify this to follow that same pattern with fastify.

@smaye81
Copy link
Member

smaye81 commented Mar 21, 2023

Hi @akinorimizushima. On second thought, we decided to combine these changes into #463 since that PR was already doing a massive refactoring of the Node examples. Your work on fastify was ported to that PR, so I am going to close this in favor of #463. Thank you for the contribution!

@smaye81 smaye81 closed this Mar 21, 2023
@akinorimizushima akinorimizushima deleted the feature/fastify-cors branch March 21, 2023 23:47
smaye81 added a commit that referenced this pull request Mar 22, 2023
This PR refactors the Connect for Node.js examples. The major changes
are:

- The examples in the `/node.js` directory have all been moved to the
top-level along with the other projects.
- Each Node example now contains a full CORS setup so that the project
can be run with a separate web interface using CORS.
- In addition, the examples also server the frontend interface via Node,
so they can be accessed from a web browser without the need for CORS.

Thanks to @akinorimizushima and his work on #444, which inspired this
CORS refactoring. The `fastify` example in #444 has been ported to this
PR.
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.

4 participants