Closed tbrannam closed 4 years ago
Hey @tbrannam, looks like an amazing work, I'll have deep look tomorrow.
Having support for hooks is great, I think we'll have to create a major release, since we're no longer supporting react@0.14
and react@15
, but it should be fine, we can potentially add it to README.
I'm sorry about the late reply (currently drowning in reviews 😅), thank you for submitting this pull request! 🎉
It's all good - I think I did everything I wanted to do - just added tests for hooks.
Think I've addressed all the outstanding issues.
One thing that I wonder about revisiting is the need to preregister variants. This is due to the Hook not having any reference to the possible list of variants like the Components would have. I like the hook interface now, but it might work if somehow the variants were passed to the hook as well as an alternative calling pattern.
Or maybe it isn't too bad the way it is?
friendly bump
I had another deep look at source code and I think it looks really good.
One way to solve the last problem that you mentioned would be to extend the useExperiment
hook, eg. use an object for advance configuration:
useExperiment('experiment_name', {
variants: ['A', 'B'],
defaultVariant: 'A',
userIdentifier: 'pk_123',
});
I think we could potentially merge this PR as it is, release a beta version of v3, test it for a few days and see how it goes, what do you think?
Sounds good
From: Paolo Moretti notifications@github.com Sent: Tuesday, September 17, 2019 5:32:42 AM To: marvelapp/react-ab-test react-ab-test@noreply.github.com Cc: Todd Brannam tbrannam@casualpenguin.com; Mention mention@noreply.github.com Subject: Re: [marvelapp/react-ab-test] Hooks refactor (#10)
I had another deep look at source code and I think it looks really good.
One way to solve the last problem that you mentioned would be to extend the useExperiment hook, eg. use an object for advance configuration:
useExperiment('experiment_name', { variants: ['A', 'B'], defaultVariant: 'A', userIdentifier: 'pk_123', });
I think we could potentially merge this PR as it, release a beta version of v3, test it for a few days and see how it goes, what do you think?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/marvelapp/react-ab-test/pull/10?email_source=notifications&email_token=AAATHXN7ENKIOJ3SKPSMJNLQKCP3VA5CNFSM4IUR4FW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD635R7A#issuecomment-532142332, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAATHXMIRLUYZZ3MUO5EHN3QKCP3VANCNFSM4IUR4FWQ.
Released in 3.0.0-beta.0
🎉
Table of Contents
Description
Introduce hooks support Refactor CoreExperiment to use hooks Update tests. Fix slow tests
Motivation and Context
Ready support for react 17.x Add hooks support for React Functional Components
How Has This Been Tested?
Updated tests related to params changes for core experiment. Added specific tests for hook, useExperiment
Screenshots (if appropriate):
Types of changes
Checklist: