refactor: flag resolver should use stricter types #2571

nunogois · 2022-11-30T11:14:18Z

Adding stricter types to FlagResolver can possibly help improve our DX - Help us prevent errors like typos, guide us to correctly add a flag when needed, and warn us of stray checks whenever we do a clean up at a later stage.

vercel · 2022-11-30T11:14:27Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Dec 16, 2022 at 6:31PM (UTC)

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
unleash-docs	⬜️ Ignored (Inspect)			Dec 16, 2022 at 6:31PM (UTC)

nunogois · 2022-11-30T11:15:36Z

src/lib/routes/admin-api/index.ts

@@ -112,6 +112,7 @@ class AdminApi extends Controller {
        this.app.use(
            '/invite-link',
            conditionalMiddleware(
+                // @ts-expect-error
                () => config.flagResolver.isEnabled('publicSignup'),


One of the strays caught by the PR - Maybe it's a check we should clean up? Or, on the other hand, correctly add to the remaining feature flags?

This should be removed. @Tymek We might have missed this one when cleaning up?

kwasniew · 2022-11-30T11:51:58Z

src/lib/types/experimental.ts

@@ -64,15 +65,17 @@ export const defaultExperimentalOptions = {

 export interface IExperimentalOptions {
    flags: {
-        [key: string]: boolean;
+        E?: boolean;


It it supposed to be some generic or a typo?

Discussed in: https://unleash-internal.slack.com/archives/C046LV6HH6W/p1669880769980979 - Needed to include this option since it's being passed in:

unleash/src/lib/create-config.ts

Lines 101 to 112 in bc3744d

function loadUI(options: IUnleashOptions): IUIConfig {

const uiO = options.ui || {};

const ui: IUIConfig = {

environment: 'Open Source',

};

ui.flags = {

E: true,

ENABLE_DARK_MODE_SUPPORT: false,

};

return mergeAll([ui, uiO]);

}

kwasniew · 2022-11-30T11:52:15Z

src/lib/util/flag-resolver.test.ts

@@ -14,9 +14,11 @@ test('should produce UI flags with extra dynamic flags', () => {
        ...defaultExperimentalOptions,
        flags: { extraFlag: false },
    };
+    // @ts-expect-error


can we get rid of those?

e.g. const resolver = new FlagResolver(config as IExperimentalOptions);

casting seems to be more idiomatic in TS tests than expecting errors

Good point, addressed in e226e30.

kwasniew

questions inline

FredrikOseberg · 2022-12-02T14:24:39Z

Before we merge this, will this break enterprise and cloud builds since they don't explicitly type? We should at least be aware that this can happen and update these in quick succession.

FredrikOseberg · 2022-12-02T14:35:54Z

Thinking a bit more about this, the IFlagKey type might be too limiting. If we only allow the flags defined in unleash, then the external flag provider has to implement an interface that only allows these flags. This can be problematic when we have flags that we want to define outside of unleash. For example, what if we wanted to have a feature flag specifically for unleash-cloud or unleash-enterprise? One specific example that comes to mind is change requests, which is a flag that is not in experimental options in unleash, because it's only used in enterprise. I think we need to consider a less restrictive type, but one that still allows the autocompletion. There should be a possibility to a looser type using the index access type of an object.

nunogois · 2022-12-05T09:51:20Z

Hey @FredrikOseberg,

Before we merge this, will this break enterprise and cloud builds since they don't explicitly type? We should at least be aware that this can happen and update these in quick succession.

A quick test locally seemed to work as intended, but it would be great if you could have a look as well just to double check.

Thinking a bit more about this, the IFlagKey type might be too limiting. If we only allow the flags defined in unleash, then the external flag provider has to implement an interface that only allows these flags. This can be problematic when we have flags that we want to define outside of unleash. For example, what if we wanted to have a feature flag specifically for unleash-cloud or unleash-enterprise? One specific example that comes to mind is change requests, which is a flag that is not in experimental options in unleash, because it's only used in enterprise. I think we need to consider a less restrictive type, but one that still allows the autocompletion. There should be a possibility to a looser type using the index access type of an object.

Seems like changeRequests is already part of experimental options in Unleash, even if it's an enterprise only feature:

unleash/src/lib/types/experimental.ts

Line 74 in 4a3d260

changeRequests?: boolean;

This seems like the right approach to me.

A looser type would also mean we would lose some things, like easily catching a stray like this: #2571 (comment)

You would still have these options:

Adding the feature flag to Unleash so it's known, which is the ideal route;
Casting your flag string as IFlagKey for specific scenarios like tests;
// @ts-expect-erroring the line, which should be avoided;

This flow seems better to me and more explicit than what we had before, WDYT?

FredrikOseberg

LGTM for now. If we discover any limitations we could always go back.

chriswk

I like that this should allow us to see which features are enabled, and what our legal keys to use are, rather than like what we have now, where you could theoretically just ask for any key.

The disadvantage is of course that we have to remember to code it in here, rather than having to just add a feature toggle in the Unleash Hosted UI. I find that I like the trade-off.

But would really like someone else to chime in as well.

Also, this looks quite like Cognite's way of doing the Rust client, where you need your feature key in an enum to be able to query for its enabled status. When writing tools like Edge and the proxy, it's a hindrance, but when using in our own controlled App, having a type that catches typos and non-existing toggles is 👍 for me

nunogois commented Nov 30, 2022

View reviewed changes

nunogois marked this pull request as ready for review November 30, 2022 11:35

nunogois requested a review from ivarconr November 30, 2022 11:35

kwasniew reviewed Nov 30, 2022

View reviewed changes

nunogois force-pushed the refactor-flagresolver-stricter-types branch from d5af045 to e226e30 Compare December 2, 2022 14:04

nunogois enabled auto-merge (squash) December 2, 2022 14:18

nunogois requested review from kwasniew and FredrikOseberg December 2, 2022 14:19

nunogois disabled auto-merge December 2, 2022 14:53

nunogois force-pushed the refactor-flagresolver-stricter-types branch from 78cb843 to b82c679 Compare December 5, 2022 11:53

nunogois force-pushed the refactor-flagresolver-stricter-types branch from b82c679 to fbdce30 Compare December 12, 2022 15:05

nunogois added 4 commits December 16, 2022 18:01

refactor: flag resolver should use stricter types

cd70ad8

address PR comments

4f3f721

fix import path

8c05aa7

refactor: DRYer approach

0186683

nunogois force-pushed the refactor-flagresolver-stricter-types branch from 0992672 to 0186683 Compare December 16, 2022 18:03

fix: update snap

ac90304

vercel bot deployed to Preview – unleash-monorepo-frontend December 16, 2022 18:26 View deployment

fix: update snap

4eed026

vercel bot deployed to Preview – unleash-monorepo-frontend December 16, 2022 18:31 View deployment

FredrikOseberg approved these changes Dec 20, 2022

View reviewed changes

chriswk approved these changes Dec 20, 2022

View reviewed changes

nunogois merged commit 7ce5b3d into main Dec 20, 2022

nunogois deleted the refactor-flagresolver-stricter-types branch December 20, 2022 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: flag resolver should use stricter types #2571

refactor: flag resolver should use stricter types #2571

nunogois commented Nov 30, 2022

vercel bot commented Nov 30, 2022 •

edited

nunogois Nov 30, 2022 •

edited

FredrikOseberg Nov 30, 2022 •

edited

kwasniew Nov 30, 2022

nunogois Dec 2, 2022

kwasniew Nov 30, 2022

kwasniew Nov 30, 2022

kwasniew Nov 30, 2022

nunogois Dec 2, 2022

kwasniew left a comment

FredrikOseberg commented Dec 2, 2022

FredrikOseberg commented Dec 2, 2022

nunogois commented Dec 5, 2022

FredrikOseberg left a comment

chriswk left a comment

	function loadUI(options: IUnleashOptions): IUIConfig {
	const uiO = options.ui \|\| {};
	const ui: IUIConfig = {
	environment: 'Open Source',
	};

	ui.flags = {
	E: true,
	ENABLE_DARK_MODE_SUPPORT: false,
	};
	return mergeAll([ui, uiO]);
	}

refactor: flag resolver should use stricter types #2571

refactor: flag resolver should use stricter types #2571

Conversation

nunogois commented Nov 30, 2022

vercel bot commented Nov 30, 2022 • edited

nunogois Nov 30, 2022 • edited

Choose a reason for hiding this comment

FredrikOseberg Nov 30, 2022 • edited

Choose a reason for hiding this comment

kwasniew Nov 30, 2022

Choose a reason for hiding this comment

nunogois Dec 2, 2022

Choose a reason for hiding this comment

kwasniew Nov 30, 2022

Choose a reason for hiding this comment

kwasniew Nov 30, 2022

Choose a reason for hiding this comment

kwasniew Nov 30, 2022

Choose a reason for hiding this comment

nunogois Dec 2, 2022

Choose a reason for hiding this comment

kwasniew left a comment

Choose a reason for hiding this comment

FredrikOseberg commented Dec 2, 2022

FredrikOseberg commented Dec 2, 2022

nunogois commented Dec 5, 2022

FredrikOseberg left a comment

Choose a reason for hiding this comment

chriswk left a comment

Choose a reason for hiding this comment

vercel bot commented Nov 30, 2022 •

edited

nunogois Nov 30, 2022 •

edited

FredrikOseberg Nov 30, 2022 •

edited