-
Notifications
You must be signed in to change notification settings - Fork 5k
ffi: wrap JSCallback objects passed as cc() function arguments #31776
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
Closed
EffortlessSteven
wants to merge
1
commit into
oven-sh:main
from
EffortlessSteven:claude/ffi-cc-jscallback-arg
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use
tempDir(..., fileTree)here instead oftempDirWithFiles.This adds another
tempDirWithFilessite in a multi-file test, but the harness already supports pre-populating fixtures viatempDir(prefix, fileTree). Please switch this setup totempDirto match the test-harness convention for new coverage in this file.Based on learnings,
tempDir(prefix, fileTree)is available here, and the test guidelines say to usetempDirfromharnessto create temporary directories.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping
tempDirWithFileshere. The other four fixture setups in this file (lines 35, 108, 208, 284) use it, andtempDiris a thin wrapper around it (harness.ts:299) whose only addition isusing-based disposal. AbeforeAll/afterAllsetup can't useusing(the directory must outlive eachit), sotempDirwould add no behavior, make this block the file's lone outlier, and use a disposable non-disposably.tempDirWithFilesis the right primitive for this pattern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a correct and well-reasoned call. The
Symbol.disposecapability thattempDiradds overtempDirWithFilesis only useful when you can actually useusing; in abeforeAll/afterAlllifecycle the directory has to stay alive across the entire describe block, sotempDirwould be a disposable used non-disposably — worse than the alternative, not better.tempDirWithFilesis the right primitive here, and staying consistent with the four existing setups in the file is the right call. I'll retract the suggestion.🐇 withdraws the sticky note
✏️ Learnings added
🧠 Learnings used