Skip to content

feat: Support Yarn PnP - #3209

Open
jclab-joseph wants to merge 1 commit into
electron:7.xfrom
jclab-joseph:feat/support-pnp
Open

jclab-joseph wants to merge 1 commit into
electron:7.xfrom
jclab-joseph:feat/support-pnp

Conversation

@jclab-joseph

@jclab-joseph jclab-joseph commented Apr 7, 2023 •

Copy link
Copy Markdown
Contributor
  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • X The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Support Yarn PnP

  • find electron/package.json with require.resolve
  • delete NODE_OPTIONS environment for start electron

@jclab-joseph
jclab-joseph requested a review from a team as a code owner April 7, 2023 03:35
@jclab-joseph jclab-joseph changed the title wip: feat: Support Yarn PnP feat: Support Yarn PnP Apr 7, 2023
Comment thread packages/api/core/src/api/start.ts Outdated
} as NodeJS.ProcessEnv,
};

delete spawnOpts.env.NODE_OPTIONS;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you explain why this is necessary? I'm a bit weary about what other effects this might have.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See below

@jclab-joseph

jclab-joseph commented Aug 9, 2023 •

Copy link
Copy Markdown
Contributor Author

@dsanders11 When executing script in package.json, .pnp.cjs is injected through NODE_OPTIONS because the pnp module loader is required.
However, this is unnecessary in electron and causes an error.

NODE_OPTIONS:

--require /.../launcher/.pnp.cjs --experimental-loader file:///.../.pnp.loader.mjs

image

Comment thread packages/api/core/src/api/start.ts Outdated
@rtritto

rtritto commented Jun 9, 2024 •

Copy link
Copy Markdown

@jclab-joseph @dsanders11 @erickzhao any update?

Related #3622

@rtritto rtritto mentioned this pull request Jun 9, 2024
5 tasks done
@rtritto

rtritto commented Jun 9, 2024 •

Copy link
Copy Markdown

@dsanders11 When executing script in package.json, .pnp.cjs is injected through NODE_OPTIONS because the pnp module loader is required. However, this is unnecessary in electron and causes an error.

NODE_OPTIONS:

--require /.../launcher/.pnp.cjs --experimental-loader file:///.../.pnp.loader.mjs

image

@dsanders11 Why PnP loader arguments aren't needed?

Currently, I get another error: #3622 (comment)

@rtritto rtritto mentioned this pull request Jun 19, 2024
3 tasks done
@rtritto

rtritto commented Jun 20, 2024

Copy link
Copy Markdown

Close #3622 in favor of this PR
Fix #3611

@rtritto

rtritto commented Jun 20, 2024 •

Copy link
Copy Markdown

@jclab-joseph @dsanders11 @erickzhao what's the problem to merge this PR?
Can I help?

@rtritto

rtritto commented Sep 20, 2024

Copy link
Copy Markdown

My diffs, still working with @electron-forge/* v7.5.0 (v7.4.0 too) stack:

@electron-forge/core
diff --git a/dist/api/start.js b/dist/api/start.js
index db36a549af102fd75d23684f5d829fd4c61dbb7b..7d55e2bb26be67b52162b8e571b56e2f023e3bf6 100644
--- a/dist/api/start.js
+++ b/dist/api/start.js
@@ -15,6 +15,13 @@ const hook_1 = require("../util/hook");
const read_package_json_1 = require("../util/read-package-json");
const resolve_dir_1 = __importDefault(require("../util/resolve-dir"));
const d = (0, debug_1.default)('electron-forge:start');
+function removePnpLoaderArguments(input) {
+    if (!input) return input;
+    return input.replace(
+        /((--require\s+[^"].+\.pnp\.cjs)|(--experimental-loader\s+[^"].+\.pnp\.loader\.mjs)|(--require\s+".+\.pnp\.cjs")|(--experimental-loader\s+".+\.pnp\.loader\.mjs")) ?/g,
+        ''
+    );
+}
exports.default = (0, tracer_1.autoTrace)({ name: 'start()', category: '@electron-forge/core' }, async (childTrace, { dir: providedDir = process.cwd(), appPath = '.', interactive = false, enableLogging = false, args = [], runAsNode = false, inspect = false, inspectBrk = false, }) => {
   const platform = process.env.npm_config_platform || process.platform;
   const arch = process.env.npm_config_arch || process.arch;
@@ -118,6 +125,7 @@ exports.default = (0, tracer_1.autoTrace)({ name: 'start()', category: '@electro
                   : {}),
           },
       };
+        spawnOpts.env.NODE_OPTIONS = removePnpLoaderArguments(spawnOpts.env.NODE_OPTIONS);
       if (runAsNode) {
           spawnOpts.env.ELECTRON_RUN_AS_NODE = 'true';
       }
@electron-forge/core-utils
diff --git a/dist/electron-version.js b/dist/electron-version.js
index 55800845e873a05b04603f55b8bcd0639a8489a6..c7beb03fa1ea0507ae735b80fa7265ba5570afe8 100644
--- a/dist/electron-version.js
+++ b/dist/electron-version.js
@@ -6,7 +6,6 @@ Object.defineProperty(exports, "__esModule", { value: true });
 exports.updateElectronDependency = exports.getElectronVersion = exports.getElectronModulePath = exports.PackageNotFoundError = void 0;
 const path_1 = __importDefault(require("path"));
 const debug_1 = __importDefault(require("debug"));
-const find_up_1 = __importDefault(require("find-up"));
 const fs_extra_1 = __importDefault(require("fs-extra"));
 const semver_1 = __importDefault(require("semver"));
 const yarn_or_npm_1 = require("./yarn-or-npm");
@@ -15,25 +14,6 @@ const electronPackageNames = ['electron-nightly', 'electron'];
 function findElectronDep(dep) {
     return electronPackageNames.includes(dep);
 }
-async function findAncestorNodeModulesPath(dir, packageName) {
-    d('Looking for a lock file to indicate the root of the repo');
-    const lockPath = await (0, find_up_1.default)(['package-lock.json', 'yarn.lock', 'pnpm-lock.yaml'], { cwd: dir, type: 'file' });
-    if (lockPath) {
-        d(`Found lock file: ${lockPath}`);
-        const nodeModulesPath = path_1.default.join(path_1.default.dirname(lockPath), 'node_modules', packageName);
-        if (await fs_extra_1.default.pathExists(nodeModulesPath)) {
-            return nodeModulesPath;
-        }
-    }
-    return Promise.resolve(undefined);
-}
-async function determineNodeModulesPath(dir, packageName) {
-    const nodeModulesPath = path_1.default.join(dir, 'node_modules', packageName);
-    if (await fs_extra_1.default.pathExists(nodeModulesPath)) {
-        return nodeModulesPath;
-    }
-    return findAncestorNodeModulesPath(dir, packageName);
-}
 class PackageNotFoundError extends Error {
     constructor(packageName, dir) {
         super(`Cannot find the package "${packageName}". Perhaps you need to run "${(0, yarn_or_npm_1.safeYarnOrNpm)()} install" in "${dir}"?`);
@@ -53,15 +33,11 @@ function getElectronModuleName(packageJSON) {
     return packageName;
 }
 async function getElectronPackageJSONPath(dir, packageName) {
-    const nodeModulesPath = await determineNodeModulesPath(dir, packageName);
-    if (!nodeModulesPath) {
+    try {
+        return require.resolve(`${packageName}/package.json`, { paths: [dir] });
+    } catch {
         throw new PackageNotFoundError(packageName, dir);
     }
-    const electronPackageJSONPath = path_1.default.join(nodeModulesPath, 'package.json');
-    if (await fs_extra_1.default.pathExists(electronPackageJSONPath)) {
-        return electronPackageJSONPath;
-    }
-    return undefined;
 }
 async function getElectronModulePath(dir, packageJSON) {
     const moduleName = getElectronModuleName(packageJSON);

@rtritto

rtritto commented Oct 1, 2024

Copy link
Copy Markdown

FYI @MarshallOfSound

@rtritto

rtritto commented Oct 4, 2024

Copy link
Copy Markdown

@erickzhao @dsanders11 please can you review?

@rtritto rtritto mentioned this pull request Oct 7, 2024
3 tasks done
@rtritto

rtritto commented Oct 21, 2024 •

Copy link
Copy Markdown

FYI @smallsung @malept

@rtritto

rtritto commented Nov 12, 2024

Copy link
Copy Markdown

@VerteDinde please can you review?

@sebbean

sebbean commented Nov 23, 2024

Copy link
Copy Markdown

@VerteDinde please can you review?

plz!

@rtritto

rtritto commented Dec 5, 2024

Copy link
Copy Markdown

@erickzhao @dsanders11 please can you review?

@rtritto

rtritto commented Jan 22, 2025

Copy link
Copy Markdown

Any update?

@rtritto

rtritto commented Feb 7, 2025

Copy link
Copy Markdown

@erikian Please can you review?

@rtritto

rtritto commented Dec 4, 2025

Copy link
Copy Markdown

@erickzhao can this PR be merged in the forge v8?

@erickzhao

Copy link
Copy Markdown
Member

Hey @rtritto, I can take another look once most of the necessary pre-requisite work is done.

As an aside, @mmaietta and our friends at Electron Builder were looking into making a standalone module collector package that might help with this + pnpm symlinks.

This branch has not been deployed

No deployments
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.

5 participants