-
Notifications
You must be signed in to change notification settings - Fork 0
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: Command and README to generate bootstrapping configuration #248
base: main
Are you sure you want to change the base?
Conversation
import { process } from './node-shim'; | ||
|
||
/** | ||
* Script to run the bootstrap-config command directly. |
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.
The command is built in the way that it is in order to be integrated into a higher-order CLI structure, thus we define the action as a Command
and separate it from actually running it.
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.
Great stuff! This will allow us to best support that server -> client use case.
README.md
Outdated
# Basic usage | ||
yarn bootstrap-config --key <sdkKey> | ||
|
||
# With custom SDK name (default is 'android') |
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.
Minor, but I wonder if a better default would be js-client-sdk
as that is likely the most common use case.
(Note that in practice, for the foreseeable future, they will be the same configurations)
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.
agreed on js-client-sdk being the best default. Especially since Android won't support this format yet.
The reason we're asking for sdkName
and version is to trigger an obfuscated or not obfuscated UFC response from the API server. Not all SDKs support both formats, so we need to be particular about specifying the correct target client.
Would it be useful to add some params to the request URL to indicate that this a "proxied" request for an offline init? Might be helpful to get a hint at this feature usage.
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.
updated the default. Added an extra param
README.md
Outdated
yarn bootstrap-config --key <sdkKey> | ||
|
||
# With custom SDK name (default is 'android') | ||
yarn bootstrap-config --key <sdkKey> --sdk js-client |
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.
Our js client actually sends the SDK name js-client-sdk
(source)
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.
✔️
README.md
Outdated
const configString = configBuilder.toString(); | ||
|
||
// Use the configuration string to initialize your SDK | ||
console.log(configString); |
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 wonder if, for our example we write to a file versus (or in addition to) outputting to the terminal; something simple like writeFileSync()
Ah I see you even support this further down with an output option s maybe just fold that into the example
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.
updated example
try { | ||
const helper = ConfigurationWireHelper.build(argv.key as string, { | ||
sdkName: argv.sdk as string, | ||
sdkVersion: 'v5.0.0', |
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 love that this is hard-coded, but it seems reasonable. Pulling from Package.json or passing it around wouldn't be great.
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.
removed hard-coding. we should be able to get by without specifying the version. Every mobile SDK version that will support configWire will also support obfuscated config (and inferring whether it is obfuscated).
Towards FF-4076
Motivation and Context
Allowing offline init in the node-server SDK, we're building up a gen2 bootstrap protocol that requires building a
ConfigurationWire
instance.Description
ConfigurationWire