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

Redux #183

Merged
merged 48 commits into from
Sep 5, 2016
Merged

Redux #183

merged 48 commits into from
Sep 5, 2016

Conversation

tomitm
Copy link
Contributor

@tomitm tomitm commented May 7, 2016

Notable changes:

  • Migration to main process
    • Proxy, UrlMapper, menu setup, and browser detection were moved into main process; too much was going on in the renderer, which actually slowed down request loads quite a bit
    • Goal is to keep most things out of the renderer and prefer to do work in main (especially if it requires remote), unless it's UI-related
    • Moved src/electron-app.js to src/main/index.js
      • There's actually no need to run main process code through babel/browserify (except for using require instead of import), since it's a node environment that supports ES6, but that's a change to be made later
  • React Router replaces windowFactory
    • TitleBar and Footer are extracted out of MainContent into an App container, which RR controls the children of
    • Home/Welcome split out of MainContent so the user isn't contained to a single browser session, and can launch as many browsers as they want. URL mappings are also shown the same way instead of the original modal
    • src/containers contains top-level route containers, leaving the smaller components that make up the containers to the original src/component
    • TitleBar uses Material-ish tab styling and shows which tab is active. Requests also have an empty state now
      • This should make it really flexible to add more features and views, such as blocking rules, options, etc.
  • Redux
    • index.js is no longer the kitchen sink! Use redux connect to allow a component to use the redux store - dispatch actions for mutations, and make use of selectors kept in src/reducers to read store data
      • The benefits of using actions and selectors are it makes it easy to understand what your app does by having all actions in one place, and by using selectors, you can make changes to the store shape without having to worry about the components. This leaves components to simply display the data that's given to them and delegate user actions to redux
    • MainContent is now RequestsContainer and no longer maintains which request is active or has a context menu (that's in Redux now)
    • Since proxy/urlMapper are in the main process, their states are synced with Redux over IPC
      • All changes should be dispatched through actions, and middleware will send them over IPC to main
      • If proxy/urlMapper have changes, they dispatch a SYNC action over IPC, which a reducer will apply to the state. This allows components to just read their states via redux's connect and not worry about how that data gets there
      • Note that to avoid excessive GC, request sync doesn't send full responses. When a request is set as active, the full response is grabbed at that time for only the single request. Was also going to send a minimal request as well, but I realised the list practically showed the whole request anyway, so that was moot
  • Misc
    • Tests now run under electron's renderer
    • Added a starting status for the proxy, so that the initial state isn't working in case it's not
    • Raven tracks actions and sends them along with error reports as breadcrumbs

screenshot

tomitm added 19 commits May 7, 2016 21:37
Open devtools with a redux action.
Initial work on redux'ing url mappings.
Split out proxy and url-mappings from index.
This means that UrlMapper is currently authoritative over redux, however in the end, redux actions should be authoritative.
No longer passing anything down from the top, components connect with redux where necessary.
index.js is no longer a kitchen sink!
…x actions and store for data.

Give Requests component the full request *container*, not just the request.
Start to deal with the circular dependency between store <> proxy/urlMapper, because both need to tell the store about updates, and the store wants to query the dbs on actions.
…to be able to manipulate them in middleware when a related action comes in, and the proxy/urlMapper need to notify the store when they have changes.

Could have resolved this by adding in an event emitter or something, as long as it could be registered after the store is setup, and reemit the events between that time.
Or keep it simple, and take advantage of JS semantics where setTimeout defers until the stack is cleared, well after store is setup.
…l-ish.

Update titlebar/footer to disable user-select, to closer resemble an application.
@tomitm tomitm mentioned this pull request Jun 4, 2016
tomitm added 10 commits June 4, 2016 23:07
Connect uses shallow-render checks to avoid re-rendering. If it's higher up, that leads to props being passed down, and components more likely to be re-rendered.
Fix Request components getting re-rendered on active/context of another request.
Fix keyboard shortcut for devtools not working.
Brutal conflict resolution; long-standing branches are horrible, kids.
# Conflicts:
#	package.json
… over IPC.

Includes a bit of a hacky workaround to make request objects serializable, since they user getters and functions.
Fix default export derp in Search and RequestDetails.
…eck).

Cleanup main process logs.
Set menu bar to auto-hide.
tomitm added 10 commits July 31, 2016 23:35
Was inconsistent with activeRequest, so that was causing some confusion.
Not much of better way to handle this. Getters on ES classes are defined as non-enumerable on the prototype. Fortunately, most request/response properties are just accessing _data, except for a handful that aren't.
Also made it a bit more efficient by combining it into a single reduce pass, rather than filter then map.
Add `proxy-get-request` to fetch the full request with response (used with `SET_ACTIVE_REQUEST`).
Revert changes to service/proxy, and move request sanitization into main/proxy instead.
Added lodash.throttle as sync is causing the UI to attempt to update more frequently than necessary.
@tomitm tomitm changed the title [WIP] Redux Redux Aug 22, 2016
tomitm added 3 commits August 22, 2016 00:41
We can speed up the build later by not running main through babel and browserify, as it's not necessary under the node environment (would just need to change import to require).
@mitchhentges
Copy link
Member

Wooh, this is a big PR. I'll CR it "soonish"

@@ -11,6 +11,8 @@ addons:
packages:
- icnsutils
- graphicsmagick
before_script:
- export DISPLAY=:99.0; sh -e /etc/init.d/xvfb start || true
Copy link
Member

Choose a reason for hiding this comment

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

Comments pls. Why 99.0? Why || true at the end? if xvfb fails to start, not sure that we should still try to run tests 'n stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, there was a reason for the || true. Should've left a comment, my bad. I'll go poke around and see if I can find out again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

|| true was to skip OSX, since Xfvb doesn't happen on there. Buut that's definitely a bad idea in case it fails on Linux, so I've made it better. 👌

tomitm added 3 commits August 27, 2016 16:01
…tch.

Change `UPDATE_BROWSER` to `DISABLE_BROWSER`, as that's what it's used for.
Abstract `push` from `react-router` with `navigateToFoo` since it's not really clear what `push` is.
Add some consistency under /containers, and make reducers vs selectors a slight bit clearer.
@mitchhentges
Copy link
Member

👍, quite a few changes, and I don't grok everything. However, it's working pretty well on Linux, and it's putting James in a good direction

@mitchhentges mitchhentges merged commit 1a57434 into james-proxy:master Sep 5, 2016
tomitm added a commit that referenced this pull request Sep 5, 2016
Weird auto merge conflict resolution between #270 and #183.
@tomitm tomitm deleted the redux branch May 14, 2017 04:14
@tomitm tomitm added this to the 2.0 Baby milestone May 14, 2017
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.

3 participants