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

feat: Support Yarn PnP #3209

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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
@@ -153,6 +153,8 @@ export default async ({
} 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

packages/api/core/src/api/start.ts Outdated Show resolved Hide resolved
@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!

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