Client and Adapter config #30
Conversation
darwin67
requested review from
djfarrelly,
jpwilliams and
tonyhb
as code owners
| @@ -27,6 +27,10 @@ data class RegistrationRequestPayload( | |||
| val v: String, | |||
| ) | |||
|
|
|||
| enum class InngestSyncResult { | |||
There was a problem hiding this comment.
ignore this for now.
was initially planning to redo the registration but didn't touch it this time.
| EventApiBaseUrl("INNGEST_BASE_URL"), | ||
| ApiBaseUrl("INNGEST_API_BASE_URL"), |
There was a problem hiding this comment.
FYI these 2 aren't in the spec, dunno if they should be https://github.com/inngest/inngest/blob/main/docs/SDK_SPEC.md#32-optional-variables
There was a problem hiding this comment.
spec is likely not fully up to date.
https://github.com/inngest/inngest-js/blob/1b9f101ca6cd310e429cebcf8bceaf2faf309624/packages/inngest/src/helpers/consts.ts#L25-L34
cc @jpwilliams @goodoldneon
There was a problem hiding this comment.
You can also see it in the docs as well.
https://www.inngest.com/docs/sdk/environment-variables
There was a problem hiding this comment.
I know about INNGEST_API_BASE_URL because I asked it to be added. Otherwise the SDK won't work with our staging environment.
| Dev("dev"), | ||
| Prod("prod"), | ||
| Other("other"), |
There was a problem hiding this comment.
On the inngest dashboard I see there's a concept of branch environments or user created environment names, would Other be able to cover all those?
There was a problem hiding this comment.
Branch should be other values.
I don't remember exactly if this should be the case. Could be a question for @jpwilliams
There was a problem hiding this comment.
actually, I think we can skip branch environments for now. that's only for Vercel integration atm, and Vercel users don't use Java.
there might be cases where we have better integration with AWS Lambda or GCP Cloud Function to simulate branch environments but we're not there yet.
| "1" -> InngestEnv.Dev | ||
| else -> { | ||
| var other = InngestEnv.Other | ||
| other.value = sysDev |
There was a problem hiding this comment.
I'm not sure if value can be set to anything besides "other" here
There was a problem hiding this comment.
There was a problem hiding this comment.
hm I guess it's just an instance variable and nothing is stopping you from having separate enums with the same value or changing the value either. Never thought about that before 🤷
There was a problem hiding this comment.
yup, exactly. probably not the best way to use it, so if we figure better ways in the future, we can swap it out.
The reason I know we want an other is because some users want to run integration tests in CI using our Dev Server but want to not make it in dev mode.
Correct 👍 |
Load client configuration and serve configuration following spec and existing SDKs.