enhancement (<OptimizelyFeature>): Convert to functional component that uses useFeature hook #32

jamesopti · 2020-03-11T19:08:51Z

Summary

Updates <OptimizelyFeature> to be a functional component that

Also adds support for userId and userAttribute overrides.

Addresses #30

Release

Is this a breaking change? In theory the logic should behave the same, as #28 was designed to implement the same logic as <OptimizelyFeature>.

Timing wise, when isServerSide is false, the state setting of the correct values of the feature/variables in useFeature happens in a second chained thennable, rather than the first.

I dont think this should be considered a breaking change (and thus warrant a major release), but I wanted to point it out.

jamesopti · 2020-03-11T19:09:48Z

src/Feature.spec.tsx

@@ -107,14 +107,14 @@ describe('<OptimizelyFeature>', () => {

      // while it's waiting for onReady()
      expect(component.text()).toBe('')
-      resolver.resolve({ sucess: true })
+      resolver.resolve({ success: true })


Success was misspelled in this entire test file, so all the tests were actually running as though success was false 🤦‍♂

jamesopti · 2020-03-11T19:11:40Z

src/Feature.tsx

-        variables: {},
-      }
-    }
+const FeatureComponent = (props: FeatureProps): any => {


Im not sure what Typescript wants for the return type of this functional component.

I tried a couple things but nothing pleased the withOptimizely definition of Component: React.ComponentType<P>,

@mjc1283 any ideas for how to improve this?

Would it be React.FC?

returning JSX.Element instead of React.ReactNode for the children render-prop, did the trick for me locally.

export interface FeatureProps extends WithOptimizelyProps { feature: string timeout?: number autoUpdate?: boolean overrideUserId?: string overrideAttributes?: UserAttributes children: (isEnabled: boolean, variables: VariableValuesObject) => JSX.Element } const FeatureComponent: React.FC<FeatureProps> = (props: FeatureProps) => { ...

I'd have to do some research on the exact differences

Update: Stack overflow explaining the differences https://stackoverflow.com/questions/58123398/when-to-use-jsx-element-vs-reactnode-vs-reactelement

Update: the withOptimizely HOC is expecting the Component arg to be of type React.ComponentType https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L79 . So the function you supply it has be of FC or SFC

Paul the wizard!

returning JSX.Element instead of React.ReactNode for the children render-prop, did the trick for me locally.

I see that works, but I think it's semantically incorrect. props.children can return any valid react markup, for which ReactNode is correct (Doesn't have to be JSX):

type ReactNode = ReactChild | ReactFragment | ReactPortal | boolean | null | undefined;

I confirmed that the issue is with the children prop's return value, specifically.

Upon searching a little bit, it looks like I'm not the only one!

DefinitelyTyped/DefinitelyTyped#18051

The suggestion to wrap the return value in a fragment works (<>{children(...)}</>), but to me feels incorrect/clunky, as it's technically modifying the react tree.

What should we do? I suppose returning the children inside a fragment isnt the worst, but I might prefer to tsignore this.

I think my vote is to use any to get around this and leave a note:

const FeatureComponent: React.FunctionComponent<FeatureProps> = (props): any => {

@jamesopti Does the fragment workaround give better type safety than using any?

Using any works for me for now, until this is fixed upstream here DefinitelyTyped/DefinitelyTyped#18051.

If we don't want to wait and prefer better type safety we could change the withOptimizely signature

@jamesopti Does the fragment workaround give better type safety than using any?

It gives better type safety, but at the expense of changing the actual compiled code (to wrap everything in a fragment).

Considering all were doing is rendering children passed by the consumer, I feel like the type safety we gain is small (the consumers children function returns valid react code).

But maybe that is useful? I guess injecting a Fragment into the rendered tree isnt so bad either.

I went with the Fragment. Decided its the slightly lesser of 2 evils here.

circAssimilate

Great refactor! Requesting changes to add a CHANGELOG. Also, doin't think a few more tests to make sure options and overrides are being passed would hurt.

Also, re: this, I think you determined we would major release after this - are you just mentioning that it won't be because of this item? Sgtm if so.

I dont think this should be considered a breaking change (and thus warrant a major release), but I wanted to point it out.

src/Feature.tsx

circAssimilate · 2020-03-11T22:23:26Z

src/Feature.tsx

-    return children(isEnabled, variables)
-  }
-}
+  return children(isEnabled, variables)


Is there value in passing clientReady and didTimeout like useFeature has? One usecase could be the decision to show a spinner while disabled until the client is ready then to show the default case.

I like this idea. Will add it in

circAssimilate · 2020-03-11T22:27:27Z

src/Feature.tsx

+    variables,
+    clientReady,
+    didTimeout
+  ] = useFeature(feature, { timeout, autoUpdate }, { overrideUserId, overrideAttributes });


Should we add tests to make sure the options and overrides are passed through? Also, it seems alright to pass these even if they're not defined (looking at the tests above) but I think it could be better to construct an args array and not pass any values if they don't exist. Wdyt?

Should we add tests to make sure the options and overrides are passed through?

Done!

Also, it seems alright to pass these even if they're not defined (looking at the tests above) but I think it could be better to construct an args array and not pass any values if they don't exist. Wdyt?

So 2 thoughts:

Since these are positional args, you still need to account for the case where userId is not provided and attributes isnt

optimizely here is the ReactSDK instance, which specifically checks for each argument to be undefined.

As such, I think this is the best approach.

…es useFeature

circAssimilate

Great work! Thanks, James!

jamesopti · 2020-03-13T21:24:32Z

src/hooks.ts

@@ -14,7 +14,7 @@
 * limitations under the License.
 */
 import { useCallback, useContext, useEffect, useState } from 'react';
-import * as optimizely from '@optimizely/optimizely-sdk';


I think this should have caused a lint error below (using the variable name optimizely when its already declared in outer scope)

@mjc1283 - Is the tsc compiler smart enough to figure out which usage is a type?

The scopes make it work as expected but I thought the tsc linter would complain.

+1 to linting against this. In our example code we try to use optimizelySdk as the name of the import.

jamesopti commented Mar 11, 2020

View reviewed changes

circAssimilate self-requested a review March 11, 2020 22:09

circAssimilate suggested changes Mar 11, 2020

View reviewed changes

convert <OptimizelyFeature> component to functional component that us…

14f657a

…es useFeature

jamesopti force-pushed the james/feature_component_refactor branch 3 times, most recently from 22e08ba to 0f1e2f4 Compare March 13, 2020 19:27

jamesopti requested review from mjc1283, pauloptimizely and circAssimilate March 13, 2020 19:27

jamesopti force-pushed the james/feature_component_refactor branch from 0f1e2f4 to 9a65698 Compare March 13, 2020 20:05

circAssimilate approved these changes Mar 13, 2020

View reviewed changes

update tests

c57e44e

jamesopti force-pushed the james/feature_component_refactor branch from 9a65698 to c57e44e Compare March 13, 2020 21:19

jamesopti commented Mar 13, 2020

View reviewed changes

update changelog

783ba81

jamesopti force-pushed the james/feature_component_refactor branch from 9188c0b to 783ba81 Compare March 15, 2020 20:41

jamesopti merged commit 14a80f2 into master Mar 15, 2020

jamesopti deleted the james/feature_component_refactor branch March 15, 2020 22:15

enhancement (<OptimizelyFeature>): Convert to functional component that uses useFeature hook #32

enhancement (<OptimizelyFeature>): Convert to functional component that uses useFeature hook #32

Uh oh!

Conversation

jamesopti commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Release

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauloptimizely Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauloptimizely Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

circAssimilate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

circAssimilate left a comment

Choose a reason for hiding this comment

Uh oh!

jamesopti Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjc1283 Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesopti commented Mar 11, 2020 •

edited

Loading

pauloptimizely Mar 11, 2020 •

edited

Loading

pauloptimizely Mar 13, 2020 •

edited

Loading

jamesopti Mar 13, 2020 •

edited

Loading

mjc1283 Mar 16, 2020 •

edited

Loading