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

@codebyterecodebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Oct 24, 2022
@nodejs-github-botnodejs-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
@codebyterecodebytere changed the title src: expose LookupAndCompile with parameterssrc: expose LookupAndCompile with parametersOct 24, 2022
@codebyterecodebytereforce-pushed the lookup-and-compile-wrapper branch from d4d1c3a to 5f9feeeCompareOctober 26, 2022 20:04
Copy link
Member

@joyeecheungjoyeecheung 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

@codebyterecodebytereforce-pushed the lookup-and-compile-wrapper branch from 5f9feee to 3b0d0e1CompareNovember 8, 2022 11:21
@nodejsnodejs deleted a comment from nodejs-github-botNov 8, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejsnodejs deleted a comment from nodejs-github-botNov 8, 2022
@codebyterecodebytereforce-pushed the lookup-and-compile-wrapper branch from 3b0d0e1 to 1fc633aCompareNovember 8, 2022 14:22
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

This needs rebase.

@codebytere
Copy link
MemberAuthor

Superseded by #53886

Sign up for freeto 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++.embeddingIssues and PRs related to embedding Node.js in another project.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@codebytere@nodejs-github-bot@aduh95@jasnell@addaleax@joyeecheung@legendecas@juanarbol