Skip to content

src: return bool on object set and define property#977

Merged
legendecas merged 2 commits intonodejs:mainfrom
legendecas:object-set
Apr 29, 2021
Merged

src: return bool on object set and define property#977
legendecas merged 2 commits intonodejs:mainfrom
legendecas:object-set

Conversation

@legendecas
Copy link
Copy Markdown
Member

If c++ exception is disabled, there is no ergonomic way to detect
failtures on void returning.

If c++ exception is disabled, there is no ergonomic way to detect
failtures on void returning.
@legendecas
Copy link
Copy Markdown
Member Author

@nodejs/node-api it will be great for this PR to have reviews, thanks a lot :D

Copy link
Copy Markdown
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

Hi @legendecas , just a clarification that changing return type from void to bool is not breaking, correct....? I don't see it as a problem, but perhaps there is something I am missing...

@legendecas
Copy link
Copy Markdown
Member Author

@KevinEady yes, this is not a breaking change. Existing code will continue to compile and works (and there are still lots of them in the test suites), like https://github.com/nodejs/node-addon-api/blob/main/test/binding.cc#L76.

@legendecas legendecas merged commit 93f1898 into nodejs:main Apr 29, 2021
@legendecas legendecas deleted the object-set branch April 29, 2021 02:49
deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Sep 23, 2021
If c++ exception is disabled, there is no ergonomic way to detect
failtures on void returning.
deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Oct 15, 2021
If c++ exception is disabled, there is no ergonomic way to detect
failtures on void returning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants