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

refactor(app): Reconfigure app-shell remote access #3683

Merged
merged 1 commit into from
Jul 8, 2019
Merged

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Jul 8, 2019

overview

This PR fixes a bunch of memory leaks in unit tests surrounding the mocks for app-shell remote module access in app.

  • The test setup used to attach a bunch of stuff to global to mimic window in the app UI at runtime
  • Those things attached to global were generated by jest.genMockFromModule
  • The modules that jest was told to generate mocks from had too much baggage to mock cleanly without memory leaks

Rather than do that, this PR centralizes access to the window attached remote methods and then mocks that single access file. This helps the tests and is also just a better idea.

P.S. If you're interested in the background of why we attach stuff to window for remote module access, see #2374

changelog

  • refactor(app): Reconfigure app-shell remote access

review requests

  • Tests still pass
  • App still functions with app-shell remote stuff, e.g.
    • Updating robots
    • Loading initial app config into state
    • Logging from the UI
    • Redux actions between main and UI processes

In my testing, this PR fixes the heap issues in #3672, so hopefully we can get this one in and then followup with that

This should fix a bunch of memory leaks in unit tests surrounding
the mocks for these remote modules
Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

🌈 tested on 🌆

@mcous mcous merged commit ea9fb82 into edge Jul 8, 2019
@mcous mcous deleted the js_fix-leaks branch July 8, 2019 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants