Skip to content
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

add LDClientEventMap + type assertions #178

Closed
wants to merge 1 commit into from
Closed

add LDClientEventMap + type assertions #178

wants to merge 1 commit into from

Conversation

thiskevinwang
Copy link
Contributor

@thiskevinwang thiskevinwang commented Oct 3, 2019

Description

  • Added interface LDClientEventMap

  • Added type assertions

    • for ldClient.on()
    • for ldClient.off()
  • Some autoformatter whitespace/indentation fixes

(Inspired by typescript's lib.dom.d.ts)

...When using the jds sdk in VSCode

The available event listener names are now available dropdown options! ✨

Before After
Screen Shot 2019-10-03 at 8 52 22 AM Screen Shot 2019-10-03 at 9 04 14 AM

- type assertion for .on()
- type assertion for .off()
@eli-darkly
Copy link
Contributor

Unfortunately, this won't work, because you are treating "change:<FLAG-KEY>" as if it is a constant. In the doc comment for on, when we say this—

"change:FLAG-KEY": The client has received a new value for a specific flag whose key is FLAG-KEY

—it means that if you have flags whose keys are flag1 and flag2, you are allowed to listen for events called change:flag1 and change:flag2. With your changes, a statement like this will fail to compile in TypeScript:

client.on('change:flag1', () => {});

Note that any change that affects TypeScript validation of function calls really needs to be accompanied by a change to the test code in test-types.ts, to ensure that usages that were allowable in the past are still allowable. I verified that this does not work by adding the above line and running npm run check-typescript.

@thiskevinwang
Copy link
Contributor Author

Ah I see. I'm still very new to TypeScript – is there some way to type a dynamic field like that? I tried doing some research but found nothing so far. :/

@eli-darkly
Copy link
Contributor

I'm also not deeply familiar with TypeScript, but I don't think that is possible, given that you're not just checking for specific values but for essentially a regex match. That's not really the kind of thing that type systems are designed to enforce, in general, although TS does not behave like other strongly typed languages in a lot of ways already. Someone floated a proposed feature like that a few years ago, but I don't think it went anywhere.

Note that the standard event listener model in JavaScript, which is what this is, has never had any notion of event name validation— it has always been possible to listen for events that might never be generated. Also, there's no guarantee that the parameter is a string literal, it could be a variable, so this could not be a compile-time check unless you also enforced that the variable could only be set to these allowed values.

@thiskevinwang thiskevinwang deleted the add-LDClientEventMap branch October 23, 2019 12:17
LaunchDarklyCI pushed a commit that referenced this pull request Nov 6, 2019
add more general HTTP abstraction, use Promises internally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants