Add type definitions #119
Open
Add type definitions #119
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SergeAstapov
reviewed
May 5, 2022
|
|
||
| The library ships with full support for TypeScript usage with the service, helper, and test helpers. The API described above works as expected, with one additional nicety and one caveat. | ||
|
|
||
| **Nicety:** the library provides you the ability to define statically your known feature flags by using a *registry* (as you may be familiar with from the registries for Ember's services, Ember Data models, etc.). If you define your keys (in kebab case!) in a registry like this: |
There was a problem hiding this comment.
woohoo! this is awesome!
|
@chriskrycho super minor, should we update .npmignore to not include |
|
That's a great call – I’ll update accordingly in a bit! |
This allows users of the library to get TS-powered definitions for the
library without installing `@types/ember-feature-flags`, and enables us
to provide good SemVer guarantees for this library.
Interesting/notable decisions made here:
- We introduce a `FeaturesRegistry` which users can customize for
improved type safety (specifically, around catching typos on their
feature flag checks in e.g. `isEnabled` etc.). If they do not do
anything with the registry, then any `string` key is allowed for the
`setup`, `isEnabled`, `enable`, `disable`, and `get` functions as
well as the `flags` property.
- We do *not* provide types for making the template-side direct access
to properties, e.g. `{{#if this.features.someFlag}}`, work. This is
not possible to capture safely at this point in a way that will give
the desired autocomplete in templates for [Glint][1] users while also
*not* lying to users about what they can access directly *without*
going through `get` when accessing the service in JS. (When, in the
future, the library moves away from using `unknownProperty()` to
using a native proxy, we *will* be able to support that.)
For those scenarios, users will need to use `get` in the template,
which will return `unknown`. That will be fine for type safety here,
since there is no type narrowing.
[1]: https://github.com/typed-ember/glint
- This tests against the same minimum version as the existing type definitions on DefinitelyTyped, so users can upgrade to at least the version supported there, and then switch over to using this. Note that type tests do *not* use Ember Try, and therefore do not drift the lockfile. This is intentional: the emitted types here are wholly unaffected by the rest of the dependencies other than those in `@types`, and for compatibility with TS we only care about the types. This makes that part of CI run *much* faster. - The type tests allow failures against the `typescript@next` version, just as the runtime tests do for Ember Canary releases.
| } | ||
| ``` | ||
|
|
||
| This applies to all the values. If you do *not* add any keys to the `FeatureFlags` interface, the types will fall back to simply allowing any string and returning a boolean value. |
There was a problem hiding this comment.
seem it should be FeaturesRegistry instead of FeatureFlags?
| @@ -0,0 +1,11 @@ | |||
| import Helper from "@ember/component/helper"; | |||
| import { Keys } from "ember-feature-flags/services/features"; | |||
There was a problem hiding this comment.
out of curiosity, should this be import type?
| @@ -0,0 +1,11 @@ | |||
| import Helper from "@ember/component/helper"; | |||
| import { Keys } from "ember-feature-flags/services/features"; | |||
There was a problem hiding this comment.
our of curiosity, should this be import type?
There was a problem hiding this comment.
It could, but since it's in a .d.ts file they're all import type effectively.
HeroicEric
reviewed
May 16, 2022
Co-authored-by: Eric Kelly <[email protected]>
|
@chriskrycho how are things looking on this? Anything I can help with? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This allows users of the library to get TS-powered definitions for the library without installing
@types/ember-feature-flags, and enables us to provide good SemVer guarantees for this library.Interesting/notable decisions made here:
We introduce a
FeaturesRegistrywhich users can customize for improved type safety (specifically, around catching typos on their feature flag checks in e.g.isEnabledetc.). If they do not do anything with the registry, then anystringkey is allowed for thesetup,isEnabled,enable,disable, andgetfunctions as well as theflagsproperty.We do not provide types for making the template-side direct access to properties, e.g.
{{#if this.features.someFlag}}, work. This is not possible to capture safely at this point in a way that will give the desired autocomplete in templates for Glint users while also not lying to users about what they can access directly without going throughgetwhen accessing the service in JS. (When, in the future, the library moves away from usingunknownProperty()to using a native proxy, we will be able to support that.)For those scenarios, users will need to use
getin the template, which will returnunknown. That will be fine for type safety here, since there is no type narrowing.In support of actually testing this, introduce GitHub Actions (not fully configured). Also update the README!
If it's helpful/preferred (e.g. for
CHANGELOG.mdpurposes) I can pull out the GitHub Actions configuration into a separate PR.