web: mark as uncloneable when possible#3709
Conversation
This tells node that the marked instances from undici are not cloneable, so that attempts to cloning those throw `DataCloneError`.
|
I am hesitant to write a test here, as it would be like testing the behaviour of |
|
I really think having a test would be useful, add it. |
|
Why is Headers unclonable? |
|
AFAIK, any cloneable that wants to be cloned in node runtime needs to
One example you can find in node is https://github.com/nodejs/node/blob/00b2f07f9ddeb8ffd2fb2108b0ed9ffa81ea000d/lib/internal/webstreams/transfer.js#L49 I don't think the above are exposed, so undici classes are not cloneable in node from that sense. |
|
I didn't mean to close this. Sorry. |
|
How can we access those symbols from userland? |
@ronag it's because webidl platform objects cannot be cloned:
Notice that the webidl for Headers does not include serializable: [[Exposed](https://webidl.spec.whatwg.org/#Exposed)=(Window,Worker)]
interface Headers {
...
} |
|
I should've mentioned, this is more to tell Node to treat undici instances as a platform object, as this api will attach a private attribute that Node can recognize. Without it, Node will try to find the I think whether/how we make some classes cloneable in undici should be a separate and broader discussion, since it might involve whether/how Node exposes its internal symbols. |
|
I also added the test. It works fine on my local but won't be effective on current CI, as the API hasn't been released. The API is on |
Good point. I updated the naming. |
Uzlopak
left a comment
There was a problem hiding this comment.
RSLGTM
btw. cloneable or clonable, is the spelling right?
|
Yeah, it's correct. In node it's always |
|
I think we should. I was focusing on the classes that are exposed to node only. |
|
WebSocket, CloseEvent, and MessageEvent are exposed in node |
|
Yes, they've been updated within this PR. I think maybe every class under |
|
Can you please fix linting? |
|
Tagging as backportable |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-3709-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1ab238207d84196479428494d52fc922833e8e5b
# Push it to GitHub
git push --set-upstream origin backport-3709-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.xThen, create a pull request where the |
|
@jazelly would you mind sending a backport commit with the instructions above? |
|
I am happy to do it. |
* web: mark as uncloneable when possible This tells node that the marked instances from undici are not cloneable, so that attempts to cloning those throw `DataCloneError`. * test: add test cases for platform objects uncloneable * fix: move to webidl * fix: move it under webidl * fixup: rename it to markAsUncloneable * fix: mark more web instances uncloneable * fixup --------- Co-authored-by: Khafra <maitken033380023@gmail.com> (cherry picked from commit 1ab2382)
* web: mark as uncloneable when possible This tells node that the marked instances from undici are not cloneable, so that attempts to cloning those throw `DataCloneError`. * test: add test cases for platform objects uncloneable * fix: move to webidl * fix: move it under webidl * fixup: rename it to markAsUncloneable * fix: mark more web instances uncloneable * fixup --------- Co-authored-by: Khafra <maitken033380023@gmail.com> (cherry picked from commit 1ab2382)
* web: mark as uncloneable when possible This tells node that the marked instances from undici are not cloneable, so that attempts to cloning those throw `DataCloneError`. * test: add test cases for platform objects uncloneable * fix: move to webidl * fix: move it under webidl * fixup: rename it to markAsUncloneable * fix: mark more web instances uncloneable * fixup --------- Co-authored-by: Khafra <maitken033380023@gmail.com> (cherry picked from commit 1ab2382)
* web: mark as uncloneable when possible This tells node that the marked instances from undici are not cloneable, so that attempts to cloning those throw `DataCloneError`. * test: add test cases for platform objects uncloneable * fix: move to webidl * fix: move it under webidl * fixup: rename it to markAsUncloneable * fix: mark more web instances uncloneable * fixup --------- Co-authored-by: Khafra <maitken033380023@gmail.com> (cherry picked from commit 1ab2382)
Rationale
This tells node that the marked instances from undici are not cloneable, so that attempts to cloning those throw
DataCloneError.This is a follow-up work from nodejs/node#55234
Changes
Marked the following as uncloneable.
Features
Bug Fixes
Breaking Changes and Deprecations
Status