🔨 Hooks Refactor #1925

Saeris · 2019-05-17T17:44:04Z

Removed unused container styles and reorganized code to follow updated coding conventions (to be documented at a later date). Added a new useScript hook (based on https://usehooks.com/useScript/) and a hooks directory.

Removed unused container styles and reorganized code to follow updated coding conventions (to be documented at a later date). Added a new useScript hook (based on https://www.robinwieruch.de/react-hooks-fetch-data/) and a hooks directory.

Saeris · 2019-05-17T17:49:25Z

Build failure appears to be unrelated to this commit.

Appears to run fine locally. If I'm missing a step here let me know!

Whoops! Mixed up my links here~

Removed unused container styles and reorganized code to follow updated coding conventions (to be documented at a later date). Added a new useScript hook (based on https://usehooks.com/useScript/) and a hooks directory.

…code-conventions-refactor

Small example refactor showing how to use Cerebral with React Hooks

@twhite96

* Add @twhite96 as a contributor * Add iOS touch icons

CompuIves

Great!! Lots of nice cleaning, I mostly have questions about the context of Cerebral. Also, it would be better to create all new files in typescript, as we're making our move to there anyway 😄

CompuIves · 2019-05-20T14:25:26Z

packages/app/src/app/index.js

-            </ThemeProvider>
-          </ApolloProvider>
-        </Provider>,
+        <Cerebral.Provider value={controller.provide()}>


Interesting, why do we do this? Aren't we setting the context twice now?

Technically yes, we are using two providers here. This seems to be the recommended approach to migrating to mobx-react-lite, as this new provider uses the react context api, and the old way does it different somehow. Basically to use useContext we have to do it this way. Doesn't appear to be a problem at the moment, but I'm happy to do further tests before we commit to this.

This has been re-addressed in 519b214, as now it's only calling controller.provide() once and passing it's values to our providers. Once the whole refactor is complete, we can remove the old provider entirely.

packages/app/src/app/pages/Sandbox/Editor/Workspace/Advertisement/Advertisement.js

CompuIves · 2019-05-20T14:27:28Z

packages/app/src/app/pages/Sandbox/Editor/Workspace/ConnectionNotice/ConnectionNotice.js

@@ -0,0 +1,18 @@
+import React, { useContext } from 'react';


Would it be better to call this file index.js for consistency with the other files?

No, the reason why we always split index.js and Component.js is to deliberately improve readability in a contributor's code editor. It quickly becomes confusing which file you're looking at when every component is defined inside of an index.js. Additionally, we may want to export multiple components, functions, etc from a single directory using named exports. A good example of this is a component that comes paired with say a GraphQL query fragment. It makes sense to keep those files separate for readability's sake, much like how we split component definitions from css-in-js with elements.js. The goal is to break files down into small, highly readable chunks. My general philosophy is that if I have to scroll to see all the contents of a single file, it's getting too big. Sometimes that's unavoidable, but you can use your best judgement there.

Right, the only thing I'm afraid of is that we're now following two different naming schemes inside the repo (alongside two languages). For new contributors we need to be clear what the preferred way is.

packages/app/src/app/pages/Sandbox/Editor/Workspace/index.js

Removed unused container styles and reorganized code to follow updated coding conventions (to be documented at a later date). Added a new useScript hook (based on https://www.robinwieruch.de/react-hooks-fetch-data/) and a hooks directory.

Whoops! Mixed up my links here~

Small example refactor showing how to use Cerebral with React Hooks

Implemented request to have all new files be TypeScript. Added a useInterval Hook (demo here: https://codesandbox.io/s/sessiontimer-useinterval-hook-demo-0l6jq) Refactored Live workspace item and it's dependents to use hooks. Refactored SSEDownNotice to use hooks. Instead of useContext(Cerebral), now using useSignals() and useStore()

…code-conventions-refactor

…de-conventions-refactor"

Saeris · 2019-05-21T21:42:06Z

Will need some additional help testing the Live components refactor, as you can't start a live session when running the client locally.

MichaelDeBoey

Hey @Saeris!

Great work on this PR!

Just some small adjustments I would suggest you'd make 🙂

packages/app/src/app/hooks/useInterval.ts

MichaelDeBoey · 2019-05-21T23:12:16Z

packages/app/src/app/hooks/useInterval.ts

+  }, [callback]);
+
+  useEffect(() => {
+    function tick() {


This can be an arrow function

const tick = () => savedCallback.current();

packages/app/src/app/hooks/useInterval.ts

packages/app/src/app/hooks/useScript.ts

packages/app/src/app/pages/Dashboard/index.js

packages/app/src/app/pages/Sandbox/Editor/Workspace/Advertisement/Advertisement.tsx

Moved Git and CreateRepo to sub-directories of Workspace/items/GitHub, as they aren't being used elsewhere.

Saeris · 2019-05-21T23:51:22Z

@MichaelDeBoey Thanks for the suggestions! I'll consider a few of those. For reference, both of these were taken from the blog posts linked at the top of each file and slightly modified from their respective original author's source primarily to fit our linting rules.

Co-Authored-By: Michaël De Boey <[email protected]>

MichaelDeBoey

Really great job in refactoring all these components @Saeris! 🙂

I think we should close this PR tho.
This one is only getting bigger and bigger and I think it would be better to have separate PR's for each refactored component instead of this one big PR (which is getting to big right now imo) so we can review it in smaller pieces.

Like I said: really great job so keep up the good work! 🙂

You can just cherry-pick your commit from this branch in new branches so this work doesn't need to be done multiple times 🙂

CC @CompuIves @SaraVieira

.all-contributorsrc

CompuIves · 2019-05-22T11:49:03Z

This is a huge improvement over the existing code! I agree with @MichaelDeBoey, this PR is huge, and that it's a big risk merging this all in at once. We did this once before and it caused some small bugs of which some are still not fixed today. I prefer to split this up into smaller testable commits, but I understand that that might be a lot of work.

I'm okay with merging this PR in, but only if we do a lot of testing on staging before we're going to put this on prod. Here's a list of things that we'll need to test:

Change title/subtitle of sandbox
Sandbox forking
Saving files
Deploying to ZEIT
Deploying to Now
Generating a diff for a commit
Changing a file and making sure that the diff is regenerated properly
Committing & Opening a PR
Which components update when changing code (this is a performance test and to make sure that the proxy mechanism still works with hooks)
Adding/Removing Dependencies (without mouse)
CodeSandbox Live (will work with staging :))
CodeSandbox Teams Live
Adding/removing people to live session, toggling classroom, toggling all settings
Make sure that the More tab works, that analytics is tracked and that the buttons there work
What happens if we go offline

I'm also curious what @christianalfoni thinks about the state management, the main thing I want to know if this refactor will be as performant as our current solution, because that's one of the main reasons that we switched. I'm not sure how shouldComponentUpdate works in React Hooks and if it can be overridden.

MichaelDeBoey · 2019-05-22T11:56:02Z

@CompuIves Another option would be to first merge #1956 (which is an extracted part from this PR).
#1956 is the basis on which all other changes are based on in this PR.

So if that one's merged, this PR can be extracted (via git cherry-pick) in multiple smaller PRs (each component it's own PR) to make it easier to review and reason about

CompuIves · 2019-05-22T12:28:11Z

That'd be best, also leads to a cleaner git history. My points about testing still stand, but that way it will be a bit cleaner.

Another option would be to first merge #1956 (which is an extracted part from this PR).

Makes sense, I want to know how this affects shouldComponentUpdate before we can merge this. We had a substantial performance increase when switching to cerebral and its custom shouldComponentUpdate and it would be a shame if we would lose that benefit.

Saeris · 2019-05-22T14:02:19Z

Yeah apologies for the huge PR! After that mixup the other day where all my work in progress got wiped out, I figured it would be safer for me to make multiple commits for each section I ended up having to re-write.

So far in all of my manual testing, everything appears to work. Here's some of what I've tried successfully:

Advertisements still load
Can create new GitHub Repos
Changes correctly appear in the GitHub tab
More correctly shows the right button when Logged in/out, opens new window to login (can't fully test on local), and forks when logged in.
NotOwnedSandboxInfo properly displays it's data (tested on ~/s/new
Tested Netlify deployment. All buttons still work as expected, states update when you kick off a build, can launch build info modal.
And I'm in the middle of refactoring the Server workspace right now. All the scripts work, restart/reboot works, can change/edit/delete environment variables. Just needs some more styling moved from the components to elements.ts

Forking and saving still work. Haven't made changes to ProjectInfo, so changing the Sandbox Title/Subtitle should be unaffected. No changes to File Explorer, so adding/removing dependencies should be unaffected.

That leaves testing Live, Now Deployment, Commits/PRs, Offline, and performance.

As far as shouldComponentUpdate goes, I don't know if there's anything special going on under the hood that would be affected by these changes, but in the specific case of WorkspaceItem (which is untouched), before my progress got reset, I did refactor that using React.memo(), which can only compare props.

christianalfoni · 2019-05-22T14:20:42Z

Hi guys!

The refactor cleans up a lot. I have not had time to go through everything, but conceptually it works exactly the same as before. The only thing to watch out for is that hooks does not handle shouldComponentUpdate, which the HOC does.

So basically most components will probably benefit from being React.memo components, as that checks the props.... and also avoids reconciliation when there are no props. That said, it is extremely difficult to evaluate. Generally it should not be an issue as components will render individually based on state changes of the state they actually look at. But if a component high up in the tree needs to render, all the children will reconcile until it meets a React.memo or similar.

Anyways... I would drop going crazy with optimizations and if one pops up, just add React.memo.

I hope React will implement "no props, no reconcile", so when a parent causes child to reconcile it skips it when no props are passed. Cause with external state stores a surprising amount of components has no props. Anyways... this should also make the transition to Overmind simpler, as we are already in hooks world.

MichaelDeBoey · 2019-05-23T16:07:21Z

@CompuIves I've extracted all @Saeris' changes into (smaller) separate branches/PRs:

Advertisement: Refactor Advertisement to use useScript #1962
ConnectionNotice: 🔨 Refactor ConnectionNotice to use hooks #1963
Live: Add useInterval Hook, Refactor Live to use useInterval #1964 (Refactor Advertisement to use useScript #1962 needs to be merged first)
SSEDownNotice: Refactor SSEDownNotice to use hooks #1965
GitHub: 🔨 Refactor GitHub Workspace to use hooks #1968
Deployment: 🔨 Refactor Deployment to use hooks #1969
More: 🔨 Refactor More to use hooks #1970
NotOwnedSandboxInfo: 🔨 Apply coding conventions to NotOwnedSandboxInfo Workspace #1971

MichaelDeBoey · 2019-09-04T16:48:33Z

@CompuIves I thought a decision was made to not continue with major refactors just for the sake of refactoring, so I guess this one (and #1964) can be closed? 🤔

Or do you want me to fix the few conflicts in #1964 so that can be closed and this one can be merged? 🤔

christianalfoni · 2019-09-25T08:10:25Z

hm, has this PR diverged too far away from current master? Been there many times with Codesandbox and refactoring. Might be an idea to create a new PR and more incrementally introduce the concepts from this PR? @CompuIves @Saeris ?

SaraVieira · 2019-09-25T08:13:43Z

yeah, this is impossible to merge now :/

I am gonna close this and explain to @Saeris

Saeris and others added 7 commits May 17, 2019 10:51

Correct Comment Link

dba78f5

Whoops! Mixed up my links here~

🔨 Add useScript Hook, Refactor Advertisement to use useScript

22cd817

Removed unused container styles and reorganized code to follow updated coding conventions (to be documented at a later date). Added a new useScript hook (based on https://usehooks.com/useScript/) and a hooks directory.

Merge remote-tracking branch 'origin/code-conventions-refactor' into …

1c87bff

…code-conventions-refactor

🔨 Add mobx-react-lite, Refactor ConnectionNotice to use hooks

d88957c

Small example refactor showing how to use Cerebral with React Hooks

Remove analytic for deleting

a5966a5

Just added iOS Touch icons for the PWA (#1936)

3c0bf1a

* Add @twhite96 as a contributor * Add iOS touch icons

[Dashboard] Keep sidebar open when search input is focused (#1937)

fac7134

CompuIves requested changes May 20, 2019

View reviewed changes

MichaelDeBoey and others added 9 commits May 20, 2019 16:42

Refactor Cube to functional component with hooks (#1939)

5d95e25

Fix modules that declare a global variable

48c5d5d

Remove performance statement

101f04b

Correct Comment Link

a4523a0

Whoops! Mixed up my links here~

🔨 Add mobx-react-lite, Refactor ConnectionNotice to use hooks

37edbc4

Small example refactor showing how to use Cerebral with React Hooks

Merge remote-tracking branch 'origin/code-conventions-refactor' into …

a304dac

…code-conventions-refactor

Auto stash before merge of "code-conventions-refactor" and "origin/co…

e05aa29

…de-conventions-refactor"

Saeris changed the title ~~🔨 Add useScript Hook, Refactor Advertisement to use useScript~~ 🔨 Hooks Refactor May 21, 2019

MichaelDeBoey suggested changes May 21, 2019

View reviewed changes

🔨 Refactor GitHub Workspace to use hooks

1212065

Moved Git and CreateRepo to sub-directories of Workspace/items/GitHub, as they aren't being used elsewhere.

Saeris and others added 4 commits May 21, 2019 16:51

Update packages/app/src/app/pages/Dashboard/index.js

4ca14c6

Co-Authored-By: Michaël De Boey <[email protected]>

🔨 Refactor Deployment Workspace to use hooks

9afad2b

🔨 Refactor More Workspace to use hooks

fca1605

🔨 Apply coding conventions to NotOwnedSandboxInfo Workspace

2c0a836

MichaelDeBoey reviewed May 22, 2019

View reviewed changes

MichaelDeBoey mentioned this pull request May 22, 2019

Add mobx-react-lite #1956

Merged

4 tasks

CompuIves reviewed May 22, 2019

View reviewed changes

.all-contributorsrc Show resolved Hide resolved

SaraVieira closed this Sep 25, 2019

MichaelDeBoey deleted the code-conventions-refactor branch December 16, 2019 19:12

MichaelDeBoey added the 🔨 Refactor label Dec 16, 2019

🔨 Hooks Refactor #1925

🔨 Hooks Refactor #1925

Uh oh!

Conversation

Saeris commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saeris commented May 17, 2019

Uh oh!

CompuIves left a comment

Choose a reason for hiding this comment

Uh oh!

CompuIves May 20, 2019

Choose a reason for hiding this comment

Uh oh!

Saeris May 20, 2019

Choose a reason for hiding this comment

Uh oh!

Saeris May 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CompuIves May 20, 2019

Choose a reason for hiding this comment

Uh oh!

Saeris May 20, 2019

Choose a reason for hiding this comment

Uh oh!

CompuIves May 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Saeris commented May 21, 2019

Uh oh!

MichaelDeBoey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaelDeBoey May 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Saeris commented May 21, 2019

Uh oh!

MichaelDeBoey left a comment • 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!

CompuIves commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelDeBoey commented May 22, 2019

Uh oh!

CompuIves commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saeris commented May 22, 2019

Uh oh!

christianalfoni commented May 22, 2019

Uh oh!

MichaelDeBoey commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelDeBoey commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christianalfoni commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SaraVieira commented Sep 25, 2019

Uh oh!

Reviewers

Assignees

Saeris commented May 17, 2019 •

edited

Loading

MichaelDeBoey left a comment •

edited

Loading

CompuIves commented May 22, 2019 •

edited

Loading

CompuIves commented May 22, 2019 •

edited

Loading

MichaelDeBoey commented May 23, 2019 •

edited

Loading

MichaelDeBoey commented Sep 4, 2019 •

edited

Loading

christianalfoni commented Sep 25, 2019 •

edited

Loading