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

src: rename IsAnyByteSource to IsAnyBufferSource #49346

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

tniessen
Copy link
Member

The current name is somewhat confusing. There is an internal ByteSource class, which is entirely unrelated to what IsAnyByteSource() does, even though both exist in the crypto subsystem of the C++ code. ByteSource objects can also be constructed from strings, for example, for which IsAnyByteSource() returns false.

Web IDL calls the types for which this function returns true BufferSource. This type is commonly used across Web Crypto, for example. Thus, rename the function to match the Web IDL naming.

Because the function also appears to accept BufferSource objects backed by SharedArrayBuffer instances, the exact Web IDL name would be AllowSharedBufferSource, but that seems unnecessarily long, so I decided to stick with "any".

The current name is somewhat confusing. There is an internal ByteSource
class, which is entirely unrelated to what IsAnyByteSource() does, even
though both exist in the crypto subsystem of the C++ code. ByteSource
objects can also be constructed from strings, for example, for which
IsAnyByteSource() returns false.

Web IDL calls the types for which this function returns true
BufferSource. This type is commonly used across Web Crypto, for example.
Thus, rename the function to match the Web IDL naming.

Because the function also appears to accept BufferSource objects backed
by SharedArrayBuffer instances, the exact Web IDL name would be
AllowSharedBufferSource, but that seems unnecessarily long, so I decided
to stick with "any".
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 27, 2023
@tniessen tniessen added the crypto Issues and PRs related to the crypto subsystem. label Aug 27, 2023
@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 28, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@tniessen
Copy link
Member Author

CI infra is broken, see nodejs/build#3475.

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2023
@nodejs-github-bot nodejs-github-bot merged commit 43704dd into nodejs:main Aug 29, 2023
71 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 43704dd

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
The current name is somewhat confusing. There is an internal ByteSource
class, which is entirely unrelated to what IsAnyByteSource() does, even
though both exist in the crypto subsystem of the C++ code. ByteSource
objects can also be constructed from strings, for example, for which
IsAnyByteSource() returns false.

Web IDL calls the types for which this function returns true
BufferSource. This type is commonly used across Web Crypto, for example.
Thus, rename the function to match the Web IDL naming.

Because the function also appears to accept BufferSource objects backed
by SharedArrayBuffer instances, the exact Web IDL name would be
AllowSharedBufferSource, but that seems unnecessarily long, so I decided
to stick with "any".

PR-URL: #49346
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
The current name is somewhat confusing. There is an internal ByteSource
class, which is entirely unrelated to what IsAnyByteSource() does, even
though both exist in the crypto subsystem of the C++ code. ByteSource
objects can also be constructed from strings, for example, for which
IsAnyByteSource() returns false.

Web IDL calls the types for which this function returns true
BufferSource. This type is commonly used across Web Crypto, for example.
Thus, rename the function to match the Web IDL naming.

Because the function also appears to accept BufferSource objects backed
by SharedArrayBuffer instances, the exact Web IDL name would be
AllowSharedBufferSource, but that seems unnecessarily long, so I decided
to stick with "any".

PR-URL: nodejs#49346
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants