-
Notifications
You must be signed in to change notification settings - Fork 137
[Feature] Environment Configuration #1758
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
Conversation
b8972f0
to
6033e3a
Compare
@@ -0,0 +1,525 @@ | |||
import * as fs from 'fs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't have the @temporalio/client
package depends on Core. User-side env config stuff will need to go either in the worker
package, or we'll make a new package for this (e.g. @temporalio/config
).
*/ | ||
export type DataSource = { path: string } | { data: string | Buffer }; | ||
|
||
interface ClientConfigTLSObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like that type is meant to be exposed publicly, no? Same for ClientConfigProfileObject
and some others.
return fs.readFileSync(source.path); | ||
} catch (error) { | ||
throw new Error( | ||
`Failed to read file at path "${source.path}": ${error instanceof Error ? error.message : String(error)}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`Failed to read file at path "${source.path}": ${error instanceof Error ? error.message : String(error)}` | |
`Failed to read file at path "${source.path}": ${(error as Error).message ?? String(error)}` |
return clientConfigProfileFromBridge(bridgeProfile); | ||
} | ||
|
||
public toObject(): ClientConfigProfileObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need these toObject
and fromObject
methods?
* const client = new Client({ connection, namespace }); | ||
* ``` | ||
*/ | ||
public toClientConnectConfig(): ClientConnectConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that will work to instantiate a NativeConnection
/Worker
.
} | ||
} | ||
|
||
#[derive(TryIntoJs, Serialize)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Serialize
/serde
? Seems like this is all being transfered using TryIntoJs
.
Closing in favor of a pure JS implementation. Notable reasons for this:
|
What was changed
Environment configuration for Typescript SDK, using core base envconfig module
Why?
Checklist
Closes [Feature Request] Environment Configuration #1727
How was this tested:
test-envconfig.ts
Any docs updates needed?
No