Skip to content

Conversation

codebytere
Copy link
Member

Refs #44018.

This PR exposes a version of LookupAndCompile that takes parameters instead of detecting them based on module IDs.

Electron currently maintains a wrapper to LookupAndCompile, which specifically requires parameters because we pass our own modules to LookupAndCompile in several places:

and therefore need to be able to able to expose the ability to set that.

I do, however, see that Node.js recently added back a version of CompileAndCall which is effectively the same as our own CompileAndCall wrapper, so if it would be better to modify that to allow parameters I would also be happy to take that path.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@codebytere codebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Oct 24, 2022
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 24, 2022
@codebytere codebytere changed the title src: expose LookupAndCompile with parameters src: expose LookupAndCompile with parameters Oct 24, 2022
@codebytere codebytere force-pushed the lookup-and-compile-wrapper branch from d4d1c3a to 5f9feee Compare October 26, 2022 20:04
@codebytere codebytere requested a review from addaleax October 26, 2022 20:04
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Still LGTM

@codebytere codebytere force-pushed the lookup-and-compile-wrapper branch from 5f9feee to 3b0d0e1 Compare November 8, 2022 11:21
@nodejs nodejs deleted a comment from nodejs-github-bot Nov 8, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs nodejs deleted a comment from nodejs-github-bot Nov 8, 2022
@codebytere codebytere force-pushed the lookup-and-compile-wrapper branch from 3b0d0e1 to 1fc633a Compare November 8, 2022 14:22
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

This needs rebase.

@codebytere
Copy link
Member Author

Superseded by #53886

@codebytere codebytere closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants