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, deps: add nbytes library #53507

Closed
wants to merge 5 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 19, 2024

Projects that seek to implement Node.js compatible APIs (like workerd) end up needing to reproduce various bits of functionality internally in order to faithfully replicate the Node.js behaviors. This is particularly true for things like byte manipulation, base64 and hex encoding, and other low-level operations. This change proposes moving much of this low-level byte manipulation code out of nodejs/src and into a new nbytes library. Initially this new library will exist in the deps directory but the intent is to spin out a new separate repository to be its home in the future. Doing so will allow other projects to use the nbytes library with exactly the same implementation as Node.js.

This PR moves only the byte swapping, legacy base64 handling, hex encoding/decoding and force ascii code. Additional commits will move additional byte manipulation logic into the library.

The separate library repo will not be created until we've moved everything from src to nbytes that should be moved.

Removing the base64.h and base64-inl.h headers here may be a breaking change for native addons. That'll need to be looked at. If it is, feel free to mark this as a semver-major change.

@jasnell jasnell requested review from mcollina, anonrig and tniessen June 19, 2024 00:25
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/gyp
  • @nodejs/http
  • @nodejs/http2
  • @nodejs/net
  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added 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 Jun 19, 2024
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Couple of things are missing:

  • updating process.versions
  • adding dep_updaters and github workflow
  • updating maintaining-dependencies.md file

@jasnell jasnell force-pushed the beginnings-of-nbytes-library branch from 5e837f2 to 81fb3b1 Compare June 19, 2024 03:12
@jasnell
Copy link
Member Author

jasnell commented Jun 19, 2024

I've converted those into todos. However, given that there is no external repo yet these may be a bit premature. Specifically, there is no version to add to process.versions yet.... and there is no update workflow yet.

@jasnell jasnell requested a review from anonrig June 19, 2024 13:47
src/base64.h Show resolved Hide resolved
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

LGTM. node_metadata.cc needs to be updated to remove base64 version, but other than that everything is perfect.

jasnell added 3 commits June 19, 2024 07:14
Projects that seek to implement Node.js compatible APIs end up
needed to reproduce various bits of functionality internally in
order to faithfully replicate the Node.js behaviors. This is
particularly true for things like byte manipulation, base64 and
hex encoding, and other low-level operations. This change
proposes moving much of this low-level byte manipulation code
out of nodejs/src and into a new `nbytes` library. Initially this
new library will exist in the `deps` directory but the intent is
to spin out a new separate repository to be its home in the future.
Doing so will allow other projects to use the nbytes library with
exactly the same implementation as Node.js.

This commit moves only the byte swapping and legacy base64 handling
code. Additional commits will move additional byte manipulation
logic into the library.
@jasnell jasnell force-pushed the beginnings-of-nbytes-library branch from 81fb3b1 to 8201113 Compare June 19, 2024 14:16
@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell force-pushed the beginnings-of-nbytes-library branch from 8201113 to d0414df Compare June 19, 2024 14:55
@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 19, 2024
@jasnell jasnell force-pushed the beginnings-of-nbytes-library branch from d0414df to 8e1b829 Compare June 19, 2024 15:57
@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell force-pushed the beginnings-of-nbytes-library branch from 8e1b829 to 13c8029 Compare June 19, 2024 16:27
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 19, 2024

@jasnell jasnell removed the needs-ci PRs that need a full CI run. label Jun 19, 2024
deps/nbytes/nbytes.gyp Outdated Show resolved Hide resolved
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM % a nit

deps/nbytes/nbytes.gyp Outdated Show resolved Hide resolved
jasnell added a commit that referenced this pull request Jun 21, 2024
Projects that seek to implement Node.js compatible APIs end up
needed to reproduce various bits of functionality internally in
order to faithfully replicate the Node.js behaviors. This is
particularly true for things like byte manipulation, base64 and
hex encoding, and other low-level operations. This change
proposes moving much of this low-level byte manipulation code
out of nodejs/src and into a new `nbytes` library. Initially this
new library will exist in the `deps` directory but the intent is
to spin out a new separate repository to be its home in the future.
Doing so will allow other projects to use the nbytes library with
exactly the same implementation as Node.js.

This commit moves only the byte swapping and legacy base64 handling
code. Additional commits will move additional byte manipulation
logic into the library.

PR-URL: #53507
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Jun 21, 2024

Landed in d335487

@jasnell jasnell closed this Jun 21, 2024
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Projects that seek to implement Node.js compatible APIs end up
needed to reproduce various bits of functionality internally in
order to faithfully replicate the Node.js behaviors. This is
particularly true for things like byte manipulation, base64 and
hex encoding, and other low-level operations. This change
proposes moving much of this low-level byte manipulation code
out of nodejs/src and into a new `nbytes` library. Initially this
new library will exist in the `deps` directory but the intent is
to spin out a new separate repository to be its home in the future.
Doing so will allow other projects to use the nbytes library with
exactly the same implementation as Node.js.

This commit moves only the byte swapping and legacy base64 handling
code. Additional commits will move additional byte manipulation
logic into the library.

PR-URL: nodejs#53507
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Jun 25, 2024
Projects that seek to implement Node.js compatible APIs end up
needed to reproduce various bits of functionality internally in
order to faithfully replicate the Node.js behaviors. This is
particularly true for things like byte manipulation, base64 and
hex encoding, and other low-level operations. This change
proposes moving much of this low-level byte manipulation code
out of nodejs/src and into a new `nbytes` library. Initially this
new library will exist in the `deps` directory but the intent is
to spin out a new separate repository to be its home in the future.
Doing so will allow other projects to use the nbytes library with
exactly the same implementation as Node.js.

This commit moves only the byte swapping and legacy base64 handling
code. Additional commits will move additional byte manipulation
logic into the library.

PR-URL: #53507
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
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. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants