Support Node 12 and 14#1
Conversation
* use Nan::Call() (revert commit #5341887ef) * use Nan::NewInstance() * fix deprecated IsNearDeath Co-Authored-By: Aleksey Smolenchuk <aleksey@uber.com>
…ngs and symbols) only.
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #43c5fe
Actionable Suggestions - 3
-
src/weakref.cc - 3
- Broken property enumeration · Line 132-137
- Broken indexed enumeration · Line 142-146
- Wrong instantiation method · Line 182-182
Review Details
-
Files reviewed - 2 · Commit Range:
ab71c5d..c9fe85d- src/weakref.cc
- test/exports.js
-
Files skipped - 4
- .github/workflows/test.yml - Reason: Filter setting
- .travis.yml - Reason: Filter setting
- README.md - Reason: Filter setting
- appveyor.yml - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at sathish.t@celestialsys.com.
Documentation & Help
AI Code Review powered by
| UNWRAP | ||
| #if NODE_MAJOR_VERSION >= 7 | ||
| info.GetReturnValue().Set(dead ? Nan::New<Array>(0) : obj->GetPropertyNames(Nan::GetCurrentContext(), KeyCollectionMode::kIncludePrototypes, ONLY_ENUMERABLE, IndexFilter::kSkipIndices).ToLocalChecked()); | ||
| #else | ||
| info.GetReturnValue().Set(dead ? Nan::New<Array>(0) : Nan::GetPropertyNames(obj).ToLocalChecked()); | ||
| #endif |
There was a problem hiding this comment.
The WeakNamedPropertyEnumerator function incorrectly returns empty arrays when the target is dead (garbage collected). This breaks the JavaScript property enumeration contract where proxy objects should consistently enumerate their own properties regardless of target liveness. The dead ? Nan::New<Array>(0) branch should be removed to always return the proxy's actual property names, ensuring consistent behavior for downstream consumers like Object.keys(), for...in loops, and JSON serialization.
Code suggestion
Check the AI-generated fix before applying
| UNWRAP | |
| #if NODE_MAJOR_VERSION >= 7 | |
| info.GetReturnValue().Set(dead ? Nan::New<Array>(0) : obj->GetPropertyNames(Nan::GetCurrentContext(), KeyCollectionMode::kIncludePrototypes, ONLY_ENUMERABLE, IndexFilter::kSkipIndices).ToLocalChecked()); | |
| #else | |
| info.GetReturnValue().Set(dead ? Nan::New<Array>(0) : Nan::GetPropertyNames(obj).ToLocalChecked()); | |
| #endif | |
| UNWRAP | |
| #if NODE_MAJOR_VERSION >= 7 | |
| info.GetReturnValue().Set(obj->GetPropertyNames(Nan::GetCurrentContext(), KeyCollectionMode::kIncludePrototypes, ONLY_ENUMERABLE, IndexFilter::kSkipIndices).ToLocalChecked()); | |
| #else | |
| info.GetReturnValue().Set(Nan::GetPropertyNames(obj).ToLocalChecked()); | |
| #endif |
Code Review Run #43c5fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| #if NODE_MAJOR_VERSION >= 7 | ||
| info.GetReturnValue().Set(dead ? Nan::New<Array>(0) : obj->GetPropertyNames(Nan::GetCurrentContext(), KeyCollectionMode::kIncludePrototypes, static_cast<PropertyFilter> (ONLY_ENUMERABLE | SKIP_STRINGS | SKIP_SYMBOLS), IndexFilter::kIncludeIndices).ToLocalChecked()); | ||
| #else | ||
| info.GetReturnValue().Set(dead ? Nan::New<Array>(0) : Nan::GetPropertyNames(obj).ToLocalChecked()); | ||
| #endif |
There was a problem hiding this comment.
The WeakIndexedPropertyEnumerator has the same critical issue as WeakNamedPropertyEnumerator - it returns empty arrays when the target is dead, breaking indexed property enumeration. This affects array-like behavior and indexed access patterns. The conditional dead check should be removed to maintain consistent enumeration behavior.
Code suggestion
Check the AI-generated fix before applying
| #if NODE_MAJOR_VERSION >= 7 | |
| info.GetReturnValue().Set(dead ? Nan::New<Array>(0) : obj->GetPropertyNames(Nan::GetCurrentContext(), KeyCollectionMode::kIncludePrototypes, static_cast<PropertyFilter> (ONLY_ENUMERABLE | SKIP_STRINGS | SKIP_SYMBOLS), IndexFilter::kIncludeIndices).ToLocalChecked()); | |
| #else | |
| info.GetReturnValue().Set(dead ? Nan::New<Array>(0) : Nan::GetPropertyNames(obj).ToLocalChecked()); | |
| #endif | |
| #if NODE_MAJOR_VERSION >= 7 | |
| info.GetReturnValue().Set(obj->GetPropertyNames(Nan::GetCurrentContext(), KeyCollectionMode::kIncludePrototypes, static_cast<PropertyFilter> (ONLY_ENUMERABLE | SKIP_STRINGS | SKIP_SYMBOLS), IndexFilter::kIncludeIndices).ToLocalChecked()); | |
| #else | |
| info.GetReturnValue().Set(Nan::GetPropertyNames(obj).ToLocalChecked()); | |
| #endif |
Code Review Run #43c5fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| Local<Object> _target = info[0].As<Object>(); | ||
| Local<Object> _emitter = info[1].As<Object>(); | ||
| Local<Object> proxy = Nan::New<ObjectTemplate>(proxyClass)->NewInstance(); | ||
| Local<Object> proxy = Nan::NewInstance(Nan::New<ObjectTemplate>(proxyClass)).ToLocalChecked(); |
There was a problem hiding this comment.
The change from Nan::New<ObjectTemplate>(proxyClass)->NewInstance() to Nan::NewInstance(Nan::New<ObjectTemplate>(proxyClass)).ToLocalChecked() is incorrect. Nan::NewInstance expects a FunctionTemplate or Function, not an ObjectTemplate. The original pattern using ->NewInstance() on the ObjectTemplate is the correct approach for instantiating objects from ObjectTemplates in NAN.
Code suggestion
Check the AI-generated fix before applying
| Local<Object> proxy = Nan::NewInstance(Nan::New<ObjectTemplate>(proxyClass)).ToLocalChecked(); | |
| Local<Object> proxy = Nan::New<ObjectTemplate>(proxyClass)->NewInstance(); |
Code Review Run #43c5fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
4 participants
Summary by Bito
This pull request updates the weak reference implementation to support Node.js versions 12 and 14, introducing conditional compilation and refactoring property enumeration methods. It removes deprecated functionality like isNearDeath callback and adjusts the testing suite accordingly to enhance compatibility and maintainability.