Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: non-singleton EppoJSClient #166
base: typo/no-singleton
Are you sure you want to change the base?
feat: non-singleton EppoJSClient #166
Changes from 8 commits
e20ef73
50e66aa
de9f653
fa5baab
953c4e4
159df11
a73bf0c
7e5abe1
92cdd1d
c4b2717
ddab6c0
d55d23d
3c18bc9
4b8077f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 seems like it could e a pain to keep in sync. If these parameters are just a supserset, could we use the spread operator instead?
Side note: I've heard a couple of places they'd like to be able to tell client not to obfuscate, such as in development environments. So I wonder if we take this opportunity to make
isObfuscated
configurable.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.
isObfuscated
is actually a booby trap.The client doesn't obfuscate anything (it only de-obfuscates), except a couple of log fields(?). Whether configuration is obfuscated or not is determined by the server based on sdkName/sdkVersion.
isObfuscated
is a flag that says "we know that server will serve obfuscated config for my sdkName/sdkVersion pair." You can't changeisObfuscated
without things blowing up.isObfuscated
should actually be removed altogether because we haveformat
andobfuscated
fields in configurations which determine whether configuration is really obfuscated.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
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.
As per my other comment, I propose to forbid re-initialization of standalone clients (because it's buggy and it's now possible to create new clients). While we can postpone the implementation of that, I believe that
buildAndInit
should not acceptforceReinitialize
already (as removing it later will be a major change)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: it's better to add a proper
Configuration
class instead of passing untyped/unvalidated stringsThere 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 have the client returning a stringified
IConfigurationWire
so devs don't need to worry about serializing the config string.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.
In my ideal world, we have a
Configuration
class withtoString()
andfromString()
methods. This way, devs still don't need to worry about serialization but they have a stronger hint at what they are expected to pass inCheck warning on line 172 in src/i-client-config.ts
Check warning on line 172 in src/i-client-config.ts
Check warning on line 172 in src/i-client-config.ts
Check warning on line 172 in src/i-client-config.ts
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 doc string is now missing
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.
fixed
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 think storage uniqueness is based on SDK which should be fine for this use case as I don't imagine multiple instances all using same SDK key, but I wonder if we should include that in the documentation somewhere
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.
oooh I like this additional organization
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.
Are these sub-types ever used separately? Having the type split like this increases the chance of getting name clashes on the fields (which typescript won't really warn us about)
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 idea is that a dev could build different options objects then combine them for initialization. If they don't need custom event and storage options, for example, they wouldn't have those configs.
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.
Could you give an example of how you see it used in users' code?
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 think the omit/pick makes it clear what is happening
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.
These types are so similar is it worth having separate ones for the constructor vs. init? What if we just allow sdkKey or apiKey for backward compatibility? I worry having two constructor input types could be confusing
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 would actually keep
apiKey
and savesdkKey
rename for the next major bumpThere 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.
fixed up. moved sdkKey to the next major