Skip to content

Conversation

@jclab-joseph
Copy link
Contributor

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

  • 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
} as NodeJS.ProcessEnv,
};

delete spawnOpts.env.NODE_OPTIONS;
Copy link
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
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
Copy link
Contributor Author

jclab-joseph commented Aug 9, 2023

@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

@rtritto
Copy link

rtritto commented Jun 9, 2024

@jclab-joseph @dsanders11 @erickzhao any update?

Related #3622

@rtritto
Copy link

rtritto commented Jun 9, 2024

@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
Copy link

rtritto commented Jun 20, 2024

Close #3622 in favor of this PR
Fix #3611

@rtritto
Copy link

rtritto commented Jun 20, 2024

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

@rtritto
Copy link

rtritto commented Sep 20, 2024

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
Copy link

rtritto commented Oct 1, 2024

FYI @MarshallOfSound

@rtritto
Copy link

rtritto commented Oct 4, 2024

@erickzhao @dsanders11 please can you review?

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

rtritto commented Oct 21, 2024

FYI @smallsung @malept

@rtritto
Copy link

rtritto commented Nov 12, 2024

@VerteDinde please can you review?

@sebbean
Copy link

sebbean commented Nov 23, 2024

@VerteDinde please can you review?

plz!

@rtritto
Copy link

rtritto commented Dec 5, 2024

@erickzhao @dsanders11 please can you review?

@rtritto
Copy link

rtritto commented Jan 22, 2025

Any update?

@rtritto
Copy link

rtritto commented Feb 7, 2025

@erikian Please can you review?

@rtritto
Copy link

rtritto commented Dec 4, 2025

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

@erickzhao
Copy link
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.

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