Skip to content

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

Closed
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:add-throw-style-getter-setter
Closed

src: add get/set pair for unhandled rejections mode#38915
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:add-throw-style-getter-setter

Conversation

@codebytere

Copy link
Copy Markdown
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.

@codebyterecodebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Jun 3, 2021
@github-actionsgithub-actionsBot 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
codebytereforce-pushed the add-throw-style-getter-setter branch from 2a8cf56 to 92ce0beCompareJune 3, 2021 10:06
Comment threadsrc/env-inl.h Outdated
@codebytere
codebytereforce-pushed the add-throw-style-getter-setter branch from 92ce0be to c3f3fc7CompareJune 3, 2021 10:57

@bl-uebl-ue left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/env-inl.h Outdated
Comment threadsrc/env.h Outdated
@codebytere
codebytereforce-pushed the add-throw-style-getter-setter branch from 65ea8d0 to 71e0e8cCompareJune 3, 2021 12:56
@codebyterecodebytere removed the needs-ci PRs that need a full CI run. label Jun 4, 2021
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@addaleax

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@addaleax

Copy link
Copy Markdown
Member

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

@jasnell

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

Yes, exactly the point.

@codebytere

codebytere commented Jun 16, 2021

Copy link
Copy Markdown
MemberAuthor

@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
Copy Markdown
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
Copy Markdown
MemberAuthor

Closing in favor of modifying the options object directly.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@codebytere@nodejs-github-bot@addaleax@jasnell@bl-ue@VoltrexKeyva