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

Problem with https://github.com/nodejs/node-addon-api/pull/874 #1158

Closed
mhdawson opened this issue Mar 30, 2022 · 3 comments
Closed

Problem with https://github.com/nodejs/node-addon-api/pull/874 #1158

mhdawson opened this issue Mar 30, 2022 · 3 comments

Comments

@mhdawson
Copy link
Member

Issue raised in closed PR - #874

Report is that PR breaks existing code:

From @ShenHongFei

This commit breaks the following code:

    void foo (const CallbackInfo& info) {
        const Object obj = info[0].As<Object>();
        obj["xxx"].As<Boolean>();
    }

because the following methods were deleted in this commit

    /// Gets or sets an indexed property or array element.
    PropertyLValue<Value> operator[](Value index  /// Property / element index
    ) const;

    /// Gets a named property.
    MaybeOrValue<Value> operator[](
        const char* utf8name  ///< UTF-8 encoded null-terminated property name
    ) const;

    /// Gets a named property.
    MaybeOrValue<Value> operator[](
        const std::string& utf8name  ///< UTF-8 encoded property name
    ) const;

    /// Gets an indexed property or array element.
    MaybeOrValue<Value> operator[](uint32_t index  ///< Property / element index
    ) const;
@mhdawson
Copy link
Member Author

Looks to me that only these 3 were removed:

/// Gets a named property.
    MaybeOrValue<Value> operator[](
        const char* utf8name  ///< UTF-8 encoded null-terminated property name
    ) const;

    /// Gets a named property.
    MaybeOrValue<Value> operator[](
        const std::string& utf8name  ///< UTF-8 encoded property name
    ) const;

    /// Gets an indexed property or array element.
    MaybeOrValue<Value> operator[](uint32_t index  ///< Property / element index
    ) const;

@mhdawson
Copy link
Member Author

Adding those back in results in these failures:

make: Entering directory '/home/midawson/newpull/node-addon-api/test/build'
  CXX(target) Release/obj.target/binding/addon.o
In file included from ../addon.cc:3:
../../napi.h:762:25: error: ‘Napi::MaybeOrValue<Napi::Value> Napi::Object::operator[](const char*) const’ cannot be overloaded with ‘Napi::Object::PropertyLValue<std::__cxx11::basic_string<char> > Napi::Object::operator[](const char*) const’
     MaybeOrValue<Value> operator[](
                         ^~~~~~~~
../../napi.h:743:33: note: previous declaration ‘Napi::Object::PropertyLValue<std::__cxx11::basic_string<char> > Napi::Object::operator[](const char*) const’
     PropertyLValue<std::string> operator[](
                                 ^~~~~~~~
../../napi.h:767:25: error: ‘Napi::MaybeOrValue<Napi::Value> Napi::Object::operator[](const string&) const’ cannot be overloaded with ‘Napi::Object::PropertyLValue<std::__cxx11::basic_string<char> > Napi::Object::operator[](const string&) const’
     MaybeOrValue<Value> operator[](
                         ^~~~~~~~
../../napi.h:748:33: note: previous declaration ‘Napi::Object::PropertyLValue<std::__cxx11::basic_string<char> > Napi::Object::operator[](const string&) const’
     PropertyLValue<std::string> operator[](
                                 ^~~~~~~~
../../napi.h:772:25: error: ‘Napi::MaybeOrValue<Napi::Value> Napi::Object::operator[](uint32_t) const’ cannot be overloaded with ‘Napi::Object::PropertyLValue<unsigned int> Napi::Object::operator[](uint32_t) const’
     MaybeOrValue<Value> operator[](uint32_t index  ///< Property / element index
                         ^~~~~~~~
../../napi.h:753:30: note: previous declaration ‘Napi::Object::PropertyLValue<unsigned int> Napi::Object::operator[](uint32_t) const’
     PropertyLValue<uint32_t> operator[](
                            ^~~~~~~~

mhdawson added a commit to mhdawson/node-addon-api that referenced this issue Mar 31, 2022
Fixes: nodejs#1158

Revert part of nodejs#874
to avoid breaking existing code.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson
Copy link
Member Author

Submitted a PR to revert just the parts that caused the regression along with updates to the tests that fail to compoile without the fix/pass with it.

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
Fixes: nodejs/node-addon-api#1158

Revert part of nodejs/node-addon-api#874
to avoid breaking existing code.

PR-URL: nodejs/node-addon-api#1159 (review)
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>

Signed-off-by: Michael Dawson <mdawson@devrus.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
Fixes: nodejs/node-addon-api#1158

Revert part of nodejs/node-addon-api#874
to avoid breaking existing code.

PR-URL: nodejs/node-addon-api#1159 (review)
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>

Signed-off-by: Michael Dawson <mdawson@devrus.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
Fixes: nodejs/node-addon-api#1158

Revert part of nodejs/node-addon-api#874
to avoid breaking existing code.

PR-URL: nodejs/node-addon-api#1159 (review)
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>

Signed-off-by: Michael Dawson <mdawson@devrus.com>
austinli64 added a commit to austinli64/node-addon-api that referenced this issue May 9, 2023
Fixes: nodejs/node-addon-api#1158

Revert part of nodejs/node-addon-api#874
to avoid breaking existing code.

PR-URL: nodejs/node-addon-api#1159 (review)
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>

Signed-off-by: Michael Dawson <mdawson@devrus.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
Fixes: nodejs/node-addon-api#1158

Revert part of nodejs/node-addon-api#874
to avoid breaking existing code.

PR-URL: nodejs/node-addon-api#1159 (review)
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>

Signed-off-by: Michael Dawson <mdawson@devrus.com>
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 a pull request may close this issue.

1 participant