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

fix: only override valid electron module names #35915

Merged
merged 1 commit into from
Oct 6, 2022
Merged

Conversation

MarshallOfSound
Copy link
Member

Fixes #33014

Notes: Doing require('electron/*') where * is not one of main, common or renderer no longer resolves with the built-in electron module

@MarshallOfSound MarshallOfSound added semver/patch backwards-compatible bug fixes no-backport labels Oct 5, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 5, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 6, 2022
@zcbenz zcbenz merged commit e31c96a into main Oct 6, 2022
@zcbenz zcbenz deleted the MarshallOfSound-patch-1 branch October 6, 2022 10:14
@release-clerk
Copy link

release-clerk bot commented Oct 6, 2022

Release Notes Persisted

Doing require('electron/*') where * is not one of main, common or renderer no longer resolves with the built-in electron module

@RaisinTen
Copy link
Contributor

@MarshallOfSound thanks for the PR! While this does fix the issue for arbitrary suffixes after electron/, things like electron/main, electron/common and electron/renderer are still requirable in userland. I'm not sure what those modules are for because internally these are still loading electron -

get: () => require('electron')
. Should those be disallowed too or are those there for a reason we should document?
It also feels a little unexpected that we can do require('electron/renderer') in the main process given that we have -
if (process.type === 'browser') {
makeElectronModule('electron/main');
}
if (process.type === 'renderer') {
makeElectronModule('electron/renderer');
}
.

@MarshallOfSound
Copy link
Member Author

@RaisinTen those module aliases are for typescript purposes.

The types for electron/main consist of only main process modules, etc.

It is intentional they work everywhere, they're superficial for TS only.

RaisinTen added a commit to RaisinTen/electron that referenced this pull request Oct 6, 2022
electron#35915 landed without any
tests, so this change adds some. This also documents why these
variations exist.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
deepak1556 pushed a commit that referenced this pull request Oct 11, 2022
* test: add tests for valid electron module names

#35915 landed without any
tests, so this change adds some. This also documents why these
variations exist.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

* fixup! doc: rephrase comment

Signed-off-by: Darshan Sen <raisinten@gmail.com>

* fixup! test: remove "Uncaught Error:" from error regex

Signed-off-by: Darshan Sen <raisinten@gmail.com>

Signed-off-by: Darshan Sen <raisinten@gmail.com>
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* test: add tests for valid electron module names

electron#35915 landed without any
tests, so this change adds some. This also documents why these
variations exist.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

* fixup! doc: rephrase comment

Signed-off-by: Darshan Sen <raisinten@gmail.com>

* fixup! test: remove "Uncaught Error:" from error regex

Signed-off-by: Darshan Sen <raisinten@gmail.com>

Signed-off-by: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: require('electron/lol') inside an electron process requires the 'electron' module instead
6 participants