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

Kf/43 link client server in prod #44

Merged

Conversation

kevinfalting
Copy link
Collaborator

Overview

This PR allows the client to make requests across domains to the server.

Related Issues / PRs

Resolves: #18, #43

Thought Process

I've documented much of it in the related issues and as commit messages. Basically, the CORS policies set in the server's middleware were incorrect and were updated. A separate config for production was required, and the Dockerfiles were updated to accomodate these new configurations.

Testing Steps

Ensure that the client and server can communicate by running them both locally and opening up the browser console to see the events retrieved from the server.

I've already pushed both the server and client to production via flyctl, so if you open the browser console at https://eventual-client.fly.dev/ you'll see the events printed to the console from the server.

Risks

If a configuration was incorrect, CORS will block requests from being made to the server from the client.

npm install @types/node@18.17.1 --save-dev

This version is the latest LTS version as of this time.
The url is temporary until we register a domain and point it at
eventual proper.

The Config func allows for selecting the config that matches the
environment passed into it.
passing no flag still runs with local config by default.

calling flag.Parse after setting up the config is important since
we want any flags in the config to be parsed as well. Right now
the config doesn't add any flags, but it may in the future.
I originally mistakenly assumed that the Access-Control-Allow-Origin
header could handle a comma separated list of origins, which turned
out to be false. The browser expects only one, and it should match
it's own origin. This meant that I had to refactor this middleware
to be able to tell if the origin was in a list of acceptable origins.

Additionally, I decided to support wildcarded subdomains because
I anticipate arbitrary development deploys for testing and we can
do that by deploying them to randomly gen'd subdomains. Perhaps I
_should_ be saying yagni, but right now I'm saying yolo. If we
decide not to support this, it's simple enough to rip out. Simply
remove the following snippet and update the function comment.

```go
for allowedOrigin := range origins {
	if !strings.HasPrefix(allowedOrigin, "*.") {
		continue
	}

	if strings.HasSuffix(origin, allowedOrigin[1:]) {
		return true
	}
}
```
this takes advantage of a build-time capability with
create-react-app where it make available any REACT_APP_*
environment variables to the build process, and it can be
referenced via process.env.REACT_APP_*. The environment variable
will be embedded into the transpiled code.

All of that allows us to pass in a different url for the api that
can be used when making api requests. The default is to use the
same origin, which locally is handled via a proxy.
this commit can be reverted once we've wired up the ui to use
events from the server.

IIFE = Immediately Invoked Function Expression
@vercel
Copy link

vercel bot commented Sep 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
eventual ✅ Ready (Inspect) Visit Preview Sep 17, 2023 6:13pm

@kevinfalting
Copy link
Collaborator Author

kevinfalting commented Sep 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
eventual ✅ Ready (Inspect) Visit Preview Sep 17, 2023 6:13pm

Vercel deploys will not work until we setup the subdomain deploys, and I'm not entirely certain vercel is what we're going to use here. We're just trialing it for now.

@eduenez33 eduenez33 merged commit 3b3ca7d into kf/41-update-server-dockerfile Sep 21, 2023
3 checks passed
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.

2 participants