The Wayback Machine - https://web.archive.org/web/20201023141338/https://github.com/nodejs/node/pull/35754
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

util: remove instance check on empty object #35754

Open
wants to merge 3 commits into
base: master
from

Conversation

@baylesa-dev
Copy link

@baylesa-dev baylesa-dev commented Oct 22, 2020

Remove a useless check on Object descriptor. It seems like js browsers engine dont do this check.

Fixes: #35730

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Remove a useless check on Object descriptor. It seems like js browsers engine dont do this check.

Fixes: #35730
Remove a useless check on Object descriptor. It seems like js browsers engine dont do this check.

Fixes: #35730
@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 22, 2020

Hi @baylesa-dev 👋 Thanks for opening this! Could you add a test to ensure the related issue doesn't happen again if we make other changes to this code in the future?

Adrien Bayles
Write test to prevent this bug in future
@aduh95
aduh95 approved these changes Oct 22, 2020
@aduh95 aduh95 added the request-ci label Oct 22, 2020
@github-actions github-actions bot removed the request-ci label Oct 22, 2020
@codecov-io
Copy link

@codecov-io codecov-io commented Oct 22, 2020

Codecov Report

Merging #35754 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #35754      +/-   ##
==========================================
- Coverage   87.92%   87.85%   -0.07%     
==========================================
  Files         477      477              
  Lines      113090   113088       -2     
  Branches    24632    24606      -26     
==========================================
- Hits        99431    99356      -75     
- Misses       7948     8020      +72     
- Partials     5711     5712       +1     
Impacted Files Coverage Δ
lib/internal/util/inspect.js 93.41% <100.00%> (-2.26%) ⬇️
src/connect_wrap.h 25.00% <0.00%> (-75.00%) ⬇️
lib/internal/repl/history.js 88.16% <0.00%> (-4.15%) ⬇️
src/inspector_agent.cc 83.88% <0.00%> (-0.86%) ⬇️
lib/internal/encoding.js 84.06% <0.00%> (-0.53%) ⬇️
src/node_http_parser.cc 80.75% <0.00%> (-0.45%) ⬇️
lib/internal/url.js 96.12% <0.00%> (-0.41%) ⬇️
lib/_http_server.js 97.83% <0.00%> (-0.11%) ⬇️
src/module_wrap.cc 70.84% <0.00%> (+0.24%) ⬆️
@baylesa-dev
Copy link
Author

@baylesa-dev baylesa-dev commented Oct 23, 2020

@aduh95 Do you know why some checks weren't successful ? What could I do to resolve them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.