-
Notifications
You must be signed in to change notification settings - Fork 7
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: DVC-9059 pull out EnvironmentConfigManager to its own library, change nodejs build to define external pacakges #564
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
f669994
to
5351674
Compare
4493435
to
51b013c
Compare
@@ -17,12 +21,18 @@ export class EnvironmentConfigManager { | |||
private readonly requestTimeoutMS: number | |||
private readonly cdnURI: string | |||
fetchConfigPromise: Promise<void> | |||
private intervalTimeout?: NodeJS.Timeout | |||
private intervalTimeout?: any |
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 does this need to be any?
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.
basically because NodeJS.Timeout
isn't supported in webworker / browser environments, they just return a number I believe. So the strategy here was to add a setInterval
+ clearInterval
interface that would be passed in, so this needs to be an Any
if we want it to compile when used in the non-NodeJS SDKs.
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.
if you wanted to get fancy you could use a type generic for this. Type it so that the generic is what is returned by the SetIntervalInterface and then use the same generic here.
|
||
constructor( | ||
logger: DVCLogger, | ||
sdkKey: string, | ||
setConfigBuffer: SetConfigBuffer, | ||
setInterval: SetIntervalInterface, |
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.
this polling config fetcher seems very specific to the nodejs local bucketing SDK, what purpose would it serve in the worker SDK other than to fetch it once?
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.
workers do last for a while (I think I've seen 10's of minutes if not longer), we'd still want polling for them.
5351674
to
6ae5b9f
Compare
ed1fb2d
to
d811b17
Compare
07fe21a
to
9c8e5dd
Compare
adc5edf
to
3ec54d0
Compare
…dejs build to define external pacakges
3ec54d0
to
d2cf865
Compare
package.json
/external
under thebuild
step.EnvironmentConfigManager
into a generic lib to be used by the Edge Worker SDK