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.

Sign up for GitHub

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

Jump to bottom

fix(ext/console): fix inspecting iterators error. #20720

Merged
merged 8 commits into from
Nov 13, 2023
Merged

fix(ext/console): fix inspecting iterators error. #20720

merged 8 commits into from
Nov 13, 2023
+20 −21

Conversation

ghosind
Copy link
Contributor

@ghosind ghosind commented Sep 28, 2023

Fixes #19776 and #20676 .

Node implements previewEntries as C++ binding, so I moved this function to internal binding.

@ghosind
Move previewEntries to internal_binding.

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
741e1fd
@ghosind
Fix lint.

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
82d912f
@ghosind
Make type as unknown, not any.

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
6f4a992
@bartlomieju
Copy link
Member

Thanks for the PR @ghosind, it appears that this change caused an actual test failure in Node.js compat tests. Could you please take a look?

ghosind added 2 commits October 7, 2023 14:38
@ghosind
Fix Console.table prints iterators error.

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
88a0ef6
@ghosind
Merge branch 'main' into fix-inspect-iterator

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
439e0ed
@ghosind
Copy link
Contributor Author

ghosind commented Oct 7, 2023

Hi @bartlomieju ,
Console.table calls console.log and consumes the iterator. So, console.table test cases that print iterators will be failures. However, it'll throw an error and terminate the test case before this change. I've removed this log and no test error now.

@bartlomieju
Copy link
Member

@crowlKats please review

crowlKats
crowlKats approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@crowlKats
Update ext/console/01_console.js

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
37202db 
Signed-off-by: Leo Kettmeir <crowlkats@toaxl.com>
@crowlKats
Update ext/console/01_console.js

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
cfed782 
Signed-off-by: Leo Kettmeir <crowlkats@toaxl.com>
@crowlKats
Update ext/console/01_console.js

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
56fbfd5 
Signed-off-by: Leo Kettmeir <crowlkats@toaxl.com>
@crowlKats crowlKats merged commit 0209f7b into denoland:main Nov 13, 2023
@mmastrac
Copy link
Contributor

This PR unfortunately broke benchmarks on main:

running 1 test
thread 'main' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.229.0/runtime/jsruntime.rs:788:42:
called `Result::unwrap()` on an `Err` value: Specifier "ext:deno_node/internal_binding/util.ts" was not passed as an extension module and was not included in the snapshot.
stack backtrace:

mmastrac added a commit that referenced this pull request Nov 13, 2023
@mmastrac
Revert "fix(ext/console): fix inspecting iterators error. (#20720)"

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
59850b8 
This reverts commit 0209f7b.
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Nov 13, 2023
@bartlomieju
Revert "fix(ext/console): fix inspecting iterators error. (denoland#2…

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
c27d768 
…0720)"

This reverts commit 0209f7b.
@bartlomieju bartlomieju mentioned this pull request Nov 13, 2023
Merged
bartlomieju added a commit that referenced this pull request Nov 13, 2023
@bartlomieju
Revert "fix(ext/console): fix inspecting iterators error. (#20720)" (#…

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
9b9ec44 
…21191)

This reverts commit 0209f7b.

Reverting because it causes failures on `main`:
#20720 (comment)
@iuioiua iuioiua mentioned this pull request Nov 15, 2023
Closed
kt3k pushed a commit that referenced this pull request Nov 17, 2023
@ghosind @kt3k
fix(ext/console): fix inspecting iterators error. (#20720)

Verified

This commit was signed with the committer’s verified signature.
KinectTheUnknown David-Joseph Xayavong
GPG key ID: B1EA99A6C5178449
Verified
Learn about vigilant mode
7e19fee 
Fixes #19776 and #20676.
kt3k pushed a commit that referenced this pull request Nov 17, 2023
@bartlomieju @kt3k
Revert "fix(ext/console): fix inspecting iterators error. (#20720)" (#…
255b41c 
…21191)

This reverts commit 0209f7b.

Reverting because it causes failures on `main`:
#20720 (comment)
zifeo pushed a commit to metatypedev/deno that referenced this pull request Nov 22, 2023
@ghosind @zifeo
fix(ext/console): fix inspecting iterators error. (denoland#20720)
261e6ce 
Fixes denoland#19776 and denoland#20676.
zifeo pushed a commit to metatypedev/deno that referenced this pull request Nov 22, 2023
@bartlomieju @zifeo
Revert "fix(ext/console): fix inspecting iterators error. (denoland#2…
92b13d1 
…0720)" (denoland#21191)

This reverts commit 0209f7b.

Reverting because it causes failures on `main`:
denoland#20720 (comment)
crowlKats added a commit to crowlKats/deno that referenced this pull request Nov 28, 2023
@crowlKats
Revert "Revert "fix(ext/console): fix inspecting iterators error. (de…
eb2b94b 
…noland#20720)" (denoland#21191)"

This reverts commit 255b41c.
crowlKats added a commit that referenced this pull request Nov 28, 2023
@crowlKats
Reland "fix(ext/console): fix inspecting iterators error. (#20720)" (#…
32c041c 
…21370)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crowlKats crowlKats

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

deno repl: fails to inspect MapIterator or SetIterator
4 participants
@ghosind @bartlomieju @mmastrac @crowlKats