Skip to content

src: remove const qualifier on Object::GetPropertyNames and Object::InstanceOf - #992

Closed
legendecas wants to merge 1 commit into
nodejs:mainfrom
legendecas:object-const
Closed

src: remove const qualifier on Object::GetPropertyNames and Object::InstanceOf#992
legendecas wants to merge 1 commit into
nodejs:mainfrom
legendecas:object-const

Conversation

@legendecas

@legendecaslegendecas commented May 5, 2021

Copy link
Copy Markdown
Member

These operations can call into JavaScript and result in arbitrary
side effects, hence shall not be considered as const member functions.

E.g. the following examples intercepts the operations by Proxy handlers:

constobj=newProxy({foo: 0,},{ownKeys(target){target.foo++;return['foo'];},getPrototypeOf(target){target.foo++;returnObject.prototype;},});Object.getOwnPropertyNames(obj);obj.foo// => 1objinstanceofObject;obj.foo// => 2

…nstanceOf
These operations can calling into JavaScript and result in arbitrary
side effects, hence shall not be considered as `const` member functions.
@mhdawson

Copy link
Copy Markdown
Member

@legendecas we might need to consider this breaking, as code may no longer compile...

@mhdawson

Copy link
Copy Markdown
Member

There has been some discussion as to what const means. Maybe we should discuss in the next team meeting.

@legendecas

Copy link
Copy Markdown
MemberAuthor

@mhdawson Yes, this is a breaking change. Apart from the const semantics, do we have any policies on node-addon-api like semver-major changes or breaking changes?

@legendecas

Copy link
Copy Markdown
MemberAuthor

Labeled as "do not land" until we concluded with a policy regarding the breaking changes and find a consensus on const qualifier semantics on node-addon-api.

@legendecas

Copy link
Copy Markdown
MemberAuthor

Discussed at today's node-api meeting. This is quite similar to const pointers and pointer to const objects. These functions did not change the Napi::* c++ objects so it seems reasonable to keep them const.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@legendecas@mhdawson