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

src: add get/set pair for unhandled rejections mode #38915

Closed

Conversation

codebytere
Copy link
Member

This PR adds a get/set pair for unhandled rejections modes. Electron wishes to be able to set this from C++ and not just a cli flag which is the only option right now.

@codebytere codebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Jun 3, 2021
@github-actions github-actions 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 Jun 3, 2021
@codebytere codebytere force-pushed the add-throw-style-getter-setter branch from 2a8cf56 to 92ce0be Compare June 3, 2021 10:06
@codebytere codebytere force-pushed the add-throw-style-getter-setter branch from 92ce0be to c3f3fc7 Compare June 3, 2021 10:57
Copy link
Contributor

@bl-ue bl-ue left a comment

Choose a reason for hiding this comment

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

I think you meant to omit const at the end of the setter's signature.

@codebytere codebytere force-pushed the add-throw-style-getter-setter branch from 65ea8d0 to 71e0e8c Compare June 3, 2021 12:56
@codebytere codebytere removed the needs-ci PRs that need a full CI run. label Jun 4, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

addaleax commented Jun 5, 2021

Can Electron modify the options object directly? This PR would currently be adding an undocumented, untested, non-public pair of functions, which I don’t think is something we’d want to land.

@jasnell
Copy link
Member

jasnell commented Jun 7, 2021

I'd be ok with this if a reasonable test can be added for it.

@addaleax
Copy link
Member

addaleax commented Jun 7, 2021

@jasnell I would still argue that unused private APIs are things that should be removed from our source code, not added.

@jasnell
Copy link
Member

jasnell commented Jun 7, 2021

Unused by whom? Electron would be using these. Perhaps finding a way of making them a part of the supported embedder API would be a better approach.

@addaleax
Copy link
Member

addaleax commented Jun 7, 2021

Perhaps finding a way of making them a part of the supported embedder API would be a better approach.

Yes, exactly the point.

@codebytere
Copy link
Member Author

codebytere commented Jun 16, 2021

@addaleax how would you envision that differently to what's here now? I added this to approximately follow the approach I took in #35024 which was similarly embedder-facing and wasn't sure what alternate approaches might be preferable 🤔

@addaleax
Copy link
Member

how would you envision that differently to what's here now?

I guess my first question would still be whether Electron can modify the options object directly… that’s also not something in the public API, but I would expect it to be fairly consistent.

In the long run, it would probably be nice if there was a generic way to get/set options through the embedder API?

I added this to approximately follow the approach I took in #35024 which was similarly embedder-facing and wasn't sure what alternate approaches might be preferable thinking

I didn’t notice that PR, sorry – I’d revert it, tbh. I don’t think Electron gains anything from that change or from this one in its current state.

@codebytere
Copy link
Member Author

Closing in favor of modifying the options object directly.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants