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

[chore] Apply context isolation and rearrange execution.ts #376

Merged
merged 14 commits into from
Feb 9, 2023

Conversation

kyungeunni
Copy link
Contributor

@kyungeunni kyungeunni commented Feb 7, 2023

base work for

Summary

Currently, all the functions running in the main process were mangled together in the electron/execute.ts file. This makes it quite difficult to understand, hard to maintain, and a blocker for adding tests.

This PR tries to extract each function to its own file so that we could avoid the difficulties mentioned above, and make it possible to add unit tests.

Another important thing is that this PR introduces context isolation as per the security guideline provided by Electron.

Implementation details

  • Turn off node integrations and enable context isolation
  • functions in execution.ts is now divided into files under api/ directory
  • the whole ipc is no longer exposed in CommunicationContext, now limited set of window.electronAPI is exposed

How to validate this change

run test, run dev

Note to reviewers:
I'm afraid this PR is quite big to go through, I wish I could break it down into smaller chunks of PRs but it was inevitable to do it all at once as everything was connected. I recommend seeing how the communication between renderer process <-> main process has been changed by exposing selected APIs in electron/preload.ts to align with the best practices.
Also, there are new components like browserManger or syntheticsManager. Those are introduced as it requires a somewhat shared state between functions as well as to make it easier for us to mock the dependency when testing. That being said, it might be an intermediate change that is subject to change in upcoming tests PRs.

@apmmachine
Copy link
Contributor

apmmachine commented Feb 7, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-09T04:27:52.657+0000

  • Duration: 3 min 52 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /test all : Re-trigger the build for all the stages.

@kyungeunni kyungeunni marked this pull request as ready for review February 7, 2023 08:41
@kyungeunni kyungeunni changed the title [chore] Apply context isolation and rearrange execution.tsto make it more modular [chore] Apply context isolation and rearrange execution.ts Feb 7, 2023
package.json Show resolved Hide resolved
@kyungeunni kyungeunni force-pushed the explore-context-isolation branch from 1ee4dc5 to 0ea4ca8 Compare February 7, 2023 09:41
Copy link
Contributor

@justinkambic justinkambic 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 slight improvements/asks and some nit comments.

Really fantastic work on this!

electron/electron.ts Outdated Show resolved Hide resolved
electron/preload.ts Show resolved Hide resolved
electron/preload.ts Outdated Show resolved Hide resolved
electron/preload.ts Outdated Show resolved Hide resolved
electron/preload.ts Outdated Show resolved Hide resolved
src/helpers/test/mockApi.ts Show resolved Hide resolved
src/hooks/useRecordingContext.test.ts Outdated Show resolved Hide resolved
src/hooks/useSyntheticsTest.ts Outdated Show resolved Hide resolved
common/types.d.ts Outdated Show resolved Hide resolved
common/types.d.ts Outdated Show resolved Hide resolved
@kyungeunni kyungeunni force-pushed the explore-context-isolation branch 2 times, most recently from 9d70d94 to 7816b8a Compare February 8, 2023 08:30
Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

LGTM!

@kyungeunni kyungeunni force-pushed the explore-context-isolation branch from 58b2ee5 to d15380a Compare February 9, 2023 04:05
@kyungeunni kyungeunni merged commit 0a7786b into elastic:main Feb 9, 2023
@kyungeunni kyungeunni deleted the explore-context-isolation branch February 9, 2023 04:49
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