🔨 Refactored 🧠 Overmind : /editor/content/preview/index to replace Cerebral with Overmind

[chinmaychaudhary]

What kind of change does this PR introduce?

Refactors code as a part of hacktoberfest mentioned in #2621.
@Saeris @christianalfoni

What is the current behavior?

/editor/content/preview/index.tsx was using app/componentConnectors

What is the new behavior?

uses useOvermind from app/overmind

What steps did you take to test this?

Tested the Preview component by making changes in the code, adding/removing dependencies

Checklist

• Documentation
• Testing
• Ready to be merged
• Added myself to contributors table

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://codesandbox-cl-git-fork-chinmay17-refactor-editor-conten-98b27a.codesandbox1.now.sh

[chinmaychaudhary]

Hey @Saeris @christianalfoni

I need help with 2 things.

1. overmind-react: createHook types

export declare const createHook: <Config extends IConfiguration>(overmindInstance?: Overmind<Config> | undefined) => () => {
    state: import("overmind/lib/internalTypes").ResolveState<Config["state"]>;
    actions: import("overmind/lib/internalTypes").ResolveActions<Config["actions"]>;
    effects: Config["effects"] & {};
    addMutationListener: (cb: IMutationCallback) => () => void;
    reaction: (stateCallback: (state: Config["state"]) => any, updateCallback: (state: Config["state"]) => void, options?: {
        nested?: boolean | undefined;
        immediate?: boolean | undefined;
    } | undefined) => any;
};

Here, the type for state is resolved by import("overmind/lib/internalTypes").ResolveState<Config["state"]>

but in reaction's first argument, the state's type is simply set to Config["state"]. Shouldn't it also be set to import("overmind/lib/internalTypes").ResolveState<Config["state"]>?

I tried finding these definitions in Overmind's repo, but couldn't find the branch for v20.

2. [TS] The type for Sandbox

Here: https://github.com/codesandbox/codesandbox-client/blob/master/packages/common/src/types/index.ts#L287

npmDependencies: {
    [dep: string]: string;
  }

npmDependencies is defined as a dictionary but the PreviewComponent checks if there's a function keys on npmDependencies ( https://github.com/codesandbox/codesandbox-client/pull/2638/files#diff-583ed60a26e45718736e7b8b2cdcfa19L65 ).

How do I add keys type to npmDependencies?

I tried doing

npmDependencies: {
    [dep: string]: string;
    keys?: () => string[];
  }

but it doesn't work.

Because of 1. and 2., the task yarn typecheck is failing.

Any help is appreciated.

[Saeris]

Hmm, this is definitely one for @christianalfoni and @CompuIves to take a look at. The Preview component is one of the more complex parts of the app. I won't have time in the next day to help with debugging this, but if neither of them get back to you by next week I'll take a look. Thanks for taking this one on!

[chinmaychaudhary]

Thank you @Saeris for the prompt response. Will wait for the help. :)

[christianalfoni]

@chinmay17 You are perfectly right about the typing issue with Reaction. This is actually fixed in next branch of Overmind. I can update a PR with the latest version to get that fixed :) #2641 . You can go to packages/app and do yarn add overmind@next overmind-react@next to fix it as well :)

The second issue if from old mobx code. You do not have to check for keys anymore. It is the old MobxMap, which is not there anymore. So just think of it as a normal object and remove the keys check :)

[chinmaychaudhary]

@christianalfoni Thank you for helping me out here. I have updated the code accordingly. Had to fix some code in packages/app/src/app/overmind/onInitialize.ts. I changed it as per https://github.com/cerebral/overmind/blob/next/packages/node_modules/overmind/src/index.ts#L877.

Waiting for your comments. :)

[christianalfoni]

Great stuff! :) Yeah, I made the same fix on onInitialize in the other branch as well. Maybe I can just close that other PR actually and we can rather get this one in

[chinmaychaudhary]

@christianalfoni thank you for the review. Let me know if any changes are required. :)

[Saeris]

Pointed out a few things that stood out to me that should be changed. Otherwise this looks good to me but we should definitely thoroughly test this and I believe @CompuIves should definitely give this one a look over.

[Saeris]

Can we change this to an interface? Following the convention this would be IPreviewProps

Suggested change

	type Props = {
	interface IPreviewProps {

[chinmaychaudhary]

Changed. Sorry that I missed it out.

[Saeris]

Suggested change

	const PreviewComponent: React.FC<Props> = ({
	export const Preview: React.FC<IPreviewProps> = ({

Just export the component directly, no need to declare the function then re-export it at the end of the file. I'm assuming the thought behind this is to ensure that the component has a display name in the React dev tools, for which we're using a Babel plugin that handles this automatically.

[chinmaychaudhary]

Thank you. Updated. :)

[Saeris]

Suggested change

export const Preview = PreviewComponent;

Let's just remove this entirely, no need to rename the component and export this way.

[chinmaychaudhary]

done

[lbogdan]

Build for latest commit cd12f7b is at https://pr2638.build.csb.dev/s/new.

[christianalfoni]

@chinmay17 I am really sorry, but the vscodeeffect branch has a big refactor where we cleaned up this component. It does not really have any logic anymore. The branch mentioned is about to be merged so I will just have to close this one. Really sorry about this, but we had no idea we would be touching this component related to the vscodeeffect.

Really awesome work on this though, tricky one to get right and it looks great 👍 😄