crypto: add CryptoKey Symbol.toStringTag#46042
Merged
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
Collaborator
nodejs-github-bot commented Jan 1, 2023
Review requested:
|
github-actionsbot removed the request-ci This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
panvaforce-pushed the add-cryptokey-toStringTag branch from 7ea1625 to e604769Compare This comment was marked as outdated.
This comment was marked as outdated.
aduh95 approved these changes Jan 1, 2023
aduh95 added the author ready Collaborator
nodejs-github-bot commented Jan 1, 2023
panva added the semver-minor 
tniessen approved these changes Jan 1, 2023
panva added the commit-queue 22 tasks
Member
bnoordhuis commented Jan 2, 2023
I'd hold off on merging until web-platform-tests/wpt#37716 is accepted (and maybe wait a few days in case it gets reverted again.) |
MemberAuthor
panva commented Jan 2, 2023
Why? This is a QoL change that's not contradicting any existing requirement and aligns with other implementer's behaviour. Even if the WPT wouldn't get updated it won't be for its incorrectness but rather its lack of being a requirement. |
Member
bnoordhuis commented Jan 2, 2023
On the off chance that the WPT people raise valid concerns about the change. It's not a break-the-world kind of bug so I'd advise against rushing out a fix. |
MemberAuthor
panva commented Jan 2, 2023
Fine. I disagree with holding off just for the record because any valid concern would go against years of existing browser implementors having this behaviour and unlikely to be met with browser vendors' support to change. |
panva removed the commit-queue 
23 tasks
This was referenced Jan 4, 2023
MemberAuthor
panva commented Jan 18, 2023
All browsers, Deno, Bun, even widely used polyfills like PeculiarVentures/webcrypto yield the same result. IMO there's no point in waiting for WPTs to be updated. Even if they weren't updated because for some reason this does not fall under what WPTs should be checking, we'd still have no reason for misalignment. |
MemberAuthor
panva commented Jan 18, 2023
WPT was merged protest-free. |
panva added commit-queue 
nodejs-github-bot removed the commit-queue Collaborator
nodejs-github-bot commented Jan 18, 2023
Landed in 13f518f |
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
closes#45987 PR-URL: #46042Fixes: #45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
RafaelGSS added a commit that referenced this pull request Jan 20, 2023
Notable changes: * crypto: * (SEMVER-MINOR) add CryptoKey Symbol.toStringTag (Filip Skokan) [#46042](#46042) * (SEMVER-MINOR) add KeyObject Symbol.toStringTag (Filip Skokan) [#46043](#46043) * http: * (SEMVER-MINOR) join authorization headers (Marco Ippolito) [#45982](#45982) * lib: * add webstreams to Duplex.from() (Debadree Chatterjee) [#46190](#46190) * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) [#46205](#46205) PR-URL: TBD
Merged
RafaelGSS added a commit that referenced this pull request Jan 20, 2023
Notable changes: * crypto: * (SEMVER-MINOR) add CryptoKey Symbol.toStringTag (Filip Skokan) [#46042](#46042) * (SEMVER-MINOR) add KeyObject Symbol.toStringTag (Filip Skokan) [#46043](#46043) * http: * (SEMVER-MINOR) join authorization headers (Marco Ippolito) [#45982](#45982) * lib: * add webstreams to Duplex.from() (Debadree Chatterjee) [#46190](#46190) * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) [#46205](#46205) PR-URL: #46286
panva added a commit to panva/node that referenced this pull request Jan 24, 2023
closesnodejs#45987 PR-URL: nodejs#46042Fixes: nodejs#45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
panva added a commit to panva/node that referenced this pull request Jan 24, 2023
closesnodejs#45987 PR-URL: nodejs#46042Fixes: nodejs#45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Backport-PR-URL: nodejs#46340
panva added a commit to panva/node that referenced this pull request Jan 24, 2023
closesnodejs#45987 PR-URL: nodejs#46042Fixes: nodejs#45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Backport-PR-URL: nodejs#46340
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
closes#45987 PR-URL: #46042Fixes: #45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
juanarbol pushed a commit to panva/node that referenced this pull request Jan 28, 2023
closesnodejs#45987 PR-URL: nodejs#46042 Backport-PR-URL: nodejs#46340Fixes: nodejs#45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
closes#45987 PR-URL: #46042Fixes: #45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
panva added a commit to panva/node that referenced this pull request Mar 31, 2023
closesnodejs#45987 PR-URL: nodejs#46042Fixes: nodejs#45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Backport-PR-URL: nodejs#47336
panva added a commit to panva/node that referenced this pull request Mar 31, 2023
closesnodejs#45987 PR-URL: nodejs#46042Fixes: nodejs#45987 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Backport-PR-URL: nodejs#47336
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
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.
Adds
Symbol.toStringTaggetter to CryptoKey to match other implementations behaviour.Fixes: #45987