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.

Sign up for GitHub

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

Jump to bottom

feat: Support Yarn PnP #3209

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

feat: Support Yarn PnP #3209

wants to merge 1 commit into from

Conversation

jclab-joseph
Copy link
Contributor

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

  • 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
dsanders11
dsanders11 reviewed Aug 8, 2023
View reviewed changes
packages/api/core/src/api/start.ts Outdated
@@ -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 •
edited

@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

erickzhao
erickzhao requested changes Aug 18, 2023
View reviewed changes
packages/api/core/src/api/start.ts Outdated
@@ -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.

I would like this to conditionally delete the strings that Yarn adds from process.env.NODE_OPTIONS instead of clearing the entire env var.

Unfortunately, commander doesn't seem to expose any utilities to parse args strings, and util.parseArgs is only available in Node 18+, so we might need to just hack this in manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used regex instead of parse to reduce complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsanders11 dsanders11 dsanders11 left review comments

@erickzhao erickzhao erickzhao requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jclab-joseph @dsanders11 @erickzhao