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

build: add enable_remote_module build flag #19821

Merged
merged 1 commit into from Sep 18, 2019

Conversation

@miniak
Copy link
Contributor

commented Aug 19, 2019

Description of Change

Allows to remove the remote module completely at build time.

Checklist

Release Notes

Notes: no-notes

@miniak miniak added the wip label Aug 19, 2019
@miniak miniak self-assigned this Aug 19, 2019
@miniak miniak force-pushed the miniak/remote-module-flag branch 5 times, most recently from 500b352 to 022cbe1 Aug 24, 2019
Copy link
Member

left a comment

Exciting!

"lib/common/clipboard-utils.ts",
"lib/common/crash-reporter.js",
"lib/common/define-properties.ts",
"lib/common/electron-binding-setup.ts",
"lib/common/error-utils.ts",
"lib/common/is-promise.ts",
"lib/common/remote/buffer-utils.ts",
"lib/common/remote/is-promise.ts",

This comment has been minimized.

Copy link
@nornagon

nornagon Aug 26, 2019

Member

Should we figure out how to remove these JS files from the bundle when the flag is off, too?

This comment has been minimized.

Copy link
@miniak

miniak Aug 26, 2019

Author Contributor

that would be great

This comment has been minimized.

Copy link
@miniak

miniak Aug 27, 2019

Author Contributor

we should do the same for the desktop-capturer. maybe a follow up PR?

This comment has been minimized.

Copy link
@miniak

miniak Aug 30, 2019

Author Contributor

@MarshallOfSound do you have any tips how to do this?

spec-main/api-remote-spec.ts Outdated Show resolved Hide resolved
spec/api-remote-spec.js Outdated Show resolved Hide resolved
@miniak miniak force-pushed the miniak/remote-module-flag branch 4 times, most recently from 44b3c04 to 2e5cc9a Aug 26, 2019
@miniak miniak marked this pull request as ready for review Sep 11, 2019
@miniak miniak removed the wip label Sep 11, 2019
@@ -14,6 +14,6 @@ if (features.isDesktopCapturerEnabled()) {
rendererModuleList.push({ name: 'desktopCapturer', loader: () => require('./desktop-capturer') })
}

if (enableRemoteModule) {
if (features.isRemoteModuleEnabled() && enableRemoteModule) {
rendererModuleList.push({ name: 'remote', loader: () => require('./remote') })
}

This comment has been minimized.

Copy link
@nornagon

nornagon Sep 12, 2019

Member

perhaps it would be friendly to add a dummy module that prints an error if you try to use remote when it's disabled at build time?

This comment has been minimized.

Copy link
@miniak

miniak Sep 12, 2019

Author Contributor

The module is just undefined even if you disable it at runtime currently. This was done to allow detecting whether the module is available.

This comment has been minimized.

Copy link
@miniak

miniak Sep 12, 2019

Author Contributor

In the future if we do remove it from core Electron, we could do this to point users to the replacement userland module.

@@ -32,7 +32,7 @@ if (features.isDesktopCapturerEnabled()) {
})
}

if (process.isRemoteModuleEnabled) {
if (features.isRemoteModuleEnabled() && process.isRemoteModuleEnabled) {
moduleList.push({
name: 'remote',
loader: () => require('@electron/internal/renderer/api/remote')

This comment has been minimized.

Copy link
@nornagon

nornagon Sep 12, 2019

Member

here too

@miniak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@deepak1556, @codebytere can please have a look as well?

@miniak miniak force-pushed the miniak/remote-module-flag branch from 2e5cc9a to 426c730 Sep 17, 2019
@jkleinsc jkleinsc requested review from codebytere and deepak1556 Sep 17, 2019
@miniak miniak requested a review from ckerr Sep 17, 2019
Copy link
Member

left a comment

most of this is just shuffling around code, dirs and includes, so this looks good to me as-is. I'd also like to see js files removed from the bundle when remote is disabled, but i think that could potentially be done in a follow-up.

@miniak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@codebytere that’s the plan to do it in a follow up PR as it should also be done for desktopCapturer, which can also be disabled at build time

@alexeykuzmin alexeykuzmin merged commit 11cd0db into master Sep 18, 2019
15 checks passed
15 checks passed
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190917.11 succeeded
Details
electron-arm64-testing Build #20190917.11 succeeded
Details
electron-woa-testing Build #20190917.10 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Sep 18, 2019

No Release Notes

@miniak miniak deleted the miniak/remote-module-flag branch Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.