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

feat: Using Recoil JS for state management #3587

Merged
merged 133 commits into from
Jul 21, 2020
Merged

Conversation

srinaath
Copy link
Contributor

@srinaath srinaath commented Jul 7, 2020

Description

This PR is the first leg of the effort to move away from React's context API for all our state management needs and instead use Recoil JS. More unit tests to follow

Task Item

Fixes #3465 #3464

lei9444 and others added 30 commits June 24, 2020 09:25
Signed-off-by: Srinaath Ravichandran <[email protected]>

Completed port to recoil on dispatcher side till storages

Signed-off-by: Srinaath Ravichandran <[email protected]>

Stable app update commits

Signed-off-by: Srinaath Ravichandran <[email protected]>

Stable dispatcher updates

Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>

Added more dispatchers

Signed-off-by: Srinaath Ravichandran <[email protected]>

Creation flow status wrapped up

Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>

Location select tests completed

Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>

# Conflicts:
#	Composer/packages/client/__tests__/components/CreationFlow/CreateOptions/index.test.tsx
#	Composer/packages/client/__tests__/components/CreationFlow/DefineConversation/index.test.tsx
#	Composer/packages/client/__tests__/components/CreationFlow/index.test.tsx
#	Composer/packages/client/__tests__/components/header.test.tsx
#	Composer/packages/client/src/App.tsx
#	Composer/packages/client/src/Onboarding/Onboarding.tsx
#	Composer/packages/client/src/Onboarding/TeachingBubbles/TeachingBubbles.tsx
#	Composer/packages/client/src/components/CreationFlow/CreationFlow.tsx
#	Composer/packages/client/src/components/CreationFlow/LocationBrowser/LocationSelectContent.tsx
#	Composer/packages/client/src/components/ProjectTree/TriggerCreationModal.tsx
#	Composer/packages/client/src/components/TestController/TestController.tsx
#	Composer/packages/client/src/components/ToolBar/ToolBar.tsx
#	Composer/packages/client/src/pages/design/createDialogModal.tsx
#	Composer/packages/client/src/pages/design/index.tsx
#	Composer/packages/client/src/pages/home/index.tsx
#	Composer/packages/client/src/pages/language-generation/index.tsx
#	Composer/packages/client/src/pages/language-understanding/index.tsx
#	Composer/packages/client/src/pages/language-understanding/publish-luis-modal.jsx
#	Composer/packages/client/src/pages/publish/index.tsx
#	Composer/packages/client/src/pages/setting/dialog-settings/index.tsx
#	Composer/packages/client/src/pages/setting/index.tsx
#	Composer/packages/client/src/pages/setting/runtime-settings/index.tsx
#	Composer/packages/client/src/pages/skills/index.tsx
#	Composer/packages/client/src/router.tsx
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>

Design test and onbarding test completed

Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>
* test: increase 'adaptive-flow' test coverage to 77% (#3530)

* + UT: adaptive-flow-renderer/widgets

* + UT: adaptive-flow-editor/utils

* remove unref hook: useWindowDimension

* + UT: KeyboardZone

* + UT: cursorTracker

* + UT: adaptive-flow-editor/constants

* +UT: AdaptiveFlowEditor

* + UT: adaptive-flow-editor/contexts

* + UT: useEditorEventApi

* fix CI error

* + UT: NodeWrapper

* + UT: EdgeMenu

* KeyboardZone behavioror test

* change the test file structure of cursorTracker

* update a test case (#3531)

Co-authored-by: Chris Whitten <[email protected]>

* fix lgWorker test failure (#3529)

Co-authored-by: Chris Whitten <[email protected]>

Co-authored-by: zeye <[email protected]>
Co-authored-by: liweitian <[email protected]>
Co-authored-by: Chris Whitten <[email protected]>
Co-authored-by: Zhixiang Zhan <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>

Minor updates

Signed-off-by: Srinaath Ravichandran <[email protected]>

Moe updates

Signed-off-by: Srinaath Ravichandran <[email protected]>

Remove template id state

Signed-off-by: Srinaath Ravichandran <[email protected]>
Signed-off-by: Srinaath Ravichandran <[email protected]>

# Conflicts:
#	Composer/packages/client/package.json
#	Composer/packages/client/src/pages/publish/index.tsx
#	Composer/packages/client/src/pages/setting/dialog-settings/index.tsx
#	Composer/packages/client/src/pages/setting/runtime-settings/index.tsx
#	Composer/packages/client/src/store/persistence/FilePersistence.ts
Signed-off-by: Srinaath Ravichandran <[email protected]>
@zhixzhan
Copy link
Contributor

@srinaath @lei9444 I have verified LG LU and some Shell api, looks good to me.

@liweitian
Copy link
Contributor

tested creating bot flow and creating trigger flow. LGTM.

@srinaath
Copy link
Contributor Author

@srinaath @lei9444 I have verified LG LU and some Shell api, looks good to me.

Awesome @zhixzhan

@srinaath
Copy link
Contributor Author

Skills, settings and Onboarding flow has been tested by @tdurnford and me

Copy link
Contributor

@tonyanziano tonyanziano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a few minor comments, but it looks good. I'll test some of the functionality out once the built version is done.

@boydc2014
Copy link
Contributor

I think @zidaneymar and @VanyLaw was also looking on more scenarios on plugin, publishing all that, can you guys share some comments?

@srinaath srinaath changed the title feat: Using Recoil JS for state management Feat:Using Recoil JS for state management Jul 20, 2020
@srinaath srinaath changed the title Feat:Using Recoil JS for state management feat: Using Recoil JS for state management Jul 20, 2020
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work. I'm excited to get to work with the recoil state.

Please continue to think about how we can improve upon this and share with the team new patterns as they emerge.

@srinaath
Copy link
Contributor Author

srinaath commented Jul 21, 2020

@a-b-r-o-w-n Yep. Lets also use the docs in https://github.com/microsoft/BotFramework-Composer/pull/3417/files and add to these docs as we uncover new patterns. I'll also update those docs this week with things we learnt from this PR

@srinaath srinaath merged commit 748b16b into main Jul 21, 2020
@srinaath srinaath deleted the leilzh-srravich/recoil-spike branch July 21, 2020 22:23
@lei9444 lei9444 mentioned this pull request Jul 22, 2020
7 tasks
@peterbozso
Copy link
Contributor

peterbozso commented Nov 12, 2020

Hi!

I am going through the code of the client application and just stumbled upon the usage of Recoil. I am a bit surprised, because I can't really see the reason why the team choose this experimental state management library over more mature and widely known and used ones - like Redux or MobX, just to name a few. I also saw in one of the referenced issues that those two have been considered, but ultimately you decided against them in favor of Recoil.

@srinaath as I see, you have been the driving force behind this. Could you please shed some light on this decision?

Thanks,
Péter

@cwhitten
Copy link
Member

cwhitten commented Nov 12, 2020

Hello @peterbozso

React Hooks has allowed more flexibility and expressiveness when it comes to data flow and state management decisions. You'll see in our architecture that we model a redux-like unidirectional data-flow pattern, but use Recoil to isolate and manage state transitions. We did indeed evaluate various options, new and old but landed on Recoil and so far are happy with this decision. We articulated this with an assessment document here: https://github.com/microsoft/BotFramework-Composer/blob/main/docs/internal/GeoffsRecoilAssessment.md

@peterbozso
Copy link
Contributor

peterbozso commented Nov 16, 2020

Thank you @cwhitten for your quick answer! It's a really informative writing about Recoil itself. What I still struggle to see is the actual reason for choosing Recoil above other, more established alternatives like Redux or MobX.

It seems to me from the assessment that Recoil was chosen primarily because it's more lightweight than the other two and aligns better with the design of the new context API and hooks. I absolutely see the merit of this, but in case of such a big project as the Composer, I see the experimental state of Recoil a much bigger danger than its previously mentioned benefits. What if Facebook decides to scrap it? What if they change it in 1.0.0 so much that almost all the current code that depends on it needs to be changed? In my experience with the JS ecosystem, the chance of either of these is well above 0 for an experimental project. Also bugs, performance problems, etc.: all the common characteristics of beta software. Because let's admit it: that's what Recoil essentially is right now. I think it's okay to choose a library like this for an internal/pet project, but it seems to me a bit too brave to build one of the main aspects (state management) of a public-facing, production-ready application on such an unstable base.
Not mentioning the fact that orders of magnitude more people know and use the other state management libraries. Chosing one of those would have significantly lowered the barrier of entry for contributors outside of the core engineering team. I think the latter is a very important aspect to consider for an open source project where attracting contributors is always a challenge, even for very well established ones.
Unfortunately I can't find any of these points addressed in the document you referenced.

Note that I am not a pro with React myself (more like a confident intermediate :)), so please excuse me my ignorance if there's any! Please also understand that I am not trying to call out or even really challenge anybody for the decision of using of Recoil in the project. Right the contrary: I feel that my understanding of the topic is lacking and I hope somebody can point me at a direction that makes the situation clear to me, so I (and hopefully others who are reading this and are similarly clueless as I am) can learn from it.

@cwhitten
Copy link
Member

For quite some time (~years), Redux itself was built on top of the React Context API, something the React team made very clear was unstable until they rewrote the entire thing. I mention this not as a counter-argument to Facebook's future with Recoil and ongoing support, but to point out that large-scale production applications can take a dependency on something not guaranteed to ship backwards compatible changes and still survive.

In addition to the context API and better composition with hooks, React Suspense will also fundamentally change the way data gets sent to the view components and from our analysis is something Recoil is set up to adopt rather easily if you model your state appropriately and something we're looking forward to integrating once it ships in the framework.

What if Facebook pulls investment from Recoil? They may, but what is important to remember is that our architecture goes beyond the tool chosen to help model & manage state. Refactoring away from Recoil won't mean that we need to rewrite our application because the client architecture is set up in such a way that we can preserve the layering and replacing a dependency won't trickle down through the system nearly as much as you may think. It will be an expensive change, but it won't sink us. I'm optimistic we won't need to do this, though.

Bottom-line: in front-end engineering you often find yourself with the opportunity to take a bet on a new tool and it's up to you to do the due diligence to make sure it isn't a gamble, which this isn't. We made a choice and it has tradeoffs, but it is defendable, and we're OK with this for now.

lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
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.

Moving Bot specific states to use Recoil JS