Skip to content

refactor(node/buffer): update cloudflare hybrid exports - #321

Merged
anonrig merged 4 commits into
mainfrom
yagiz/update-buffer-cloudflare
Oct 10, 2024
Merged

refactor(node/buffer): update cloudflare hybrid exports#321
anonrig merged 4 commits into
mainfrom
yagiz/update-buffer-cloudflare

Conversation

@anonrig

@anonrig anonrig commented Oct 9, 2024

Copy link
Copy Markdown
Contributor

Let's reflect the truth for cloudflare buffer hybrid module. Please validate before landing this.

@anonrig
anonrig requested a review from a team as a code owner October 9, 2024 18:31
@anonrig
anonrig requested a review from jasnell October 9, 2024 18:35
@pi0

pi0 commented Oct 9, 2024

Copy link
Copy Markdown
Member

Thanks for PR. But with this, if there is a code that imports node:buffer, it will loose access to the same exports from Node.js.

$ Object.keys(require('buffer'))
[
  'Buffer',            'SlowBuffer',
  'transcode',         'isUtf8',
  'isAscii',           'kMaxLength',
  'kStringMaxLength',  'btoa',
  'atob',              'constants',
  'INSPECT_MAX_BYTES', 'Blob',
  'resolveObjectURL',  'File'
]

Can you please clarify the goal of this?

@vicb vicb added the cloudflare label Oct 9, 2024
@anonrig
anonrig force-pushed the yagiz/update-buffer-cloudflare branch from dbc2a2e to cc23c02 Compare October 9, 2024 18:36
@anonrig

anonrig commented Oct 9, 2024

Copy link
Copy Markdown
Contributor Author

I'm trying to get myself familiar with unenv repository :) I've updated the pull-request.

@anonrig
anonrig force-pushed the yagiz/update-buffer-cloudflare branch from 545685f to 3947ca2 Compare October 9, 2024 18:37
petebacondarwin
petebacondarwin reviewed Oct 9, 2024
View reviewed changes
Comment thread src/runtime/node/buffer/$cloudflare.ts
vicb
vicb requested changes Oct 9, 2024
View reviewed changes
Comment thread src/runtime/node/buffer/$cloudflare.ts
@anonrig
anonrig force-pushed the yagiz/update-buffer-cloudflare branch from 19cc3ba to 7996608 Compare October 9, 2024 18:42
pi0
pi0 approved these changes Oct 9, 2024
View reviewed changes

@pi0 pi0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❤️

@pi0 pi0 changed the title update cloudflare buffer hybrid module refactor(node/buffer): update cloudflare hybrid exports Oct 9, 2024
vicb
vicb requested changes Oct 9, 2024
View reviewed changes

@vicb vicb left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The updates looks great.

Thanks to @pi0, it's now easy to add tests (see #311).

Could you please add a test (i.e. asserting the types of the moved exports).

@anonrig

anonrig commented Oct 9, 2024

Copy link
Copy Markdown
Contributor Author

Could you please add a test (i.e. asserting the types of the moved exports).

Any suggestions on where and how to test them?

@vicb

vicb commented Oct 9, 2024

Copy link
Copy Markdown
Contributor

Any suggestions on where and how to test them?

My suggestion was to check #311 -> https://github.com/unjs/unenv/blob/main/test/workerd/tests.mjs

@anonrig
anonrig force-pushed the yagiz/update-buffer-cloudflare branch from 8ca69fe to 72d6c6c Compare October 10, 2024 15:28
@anonrig
anonrig force-pushed the yagiz/update-buffer-cloudflare branch from 72d6c6c to 990df89 Compare October 10, 2024 15:29
@anonrig
anonrig requested a review from vicb October 10, 2024 15:31
@anonrig

anonrig commented Oct 10, 2024

Copy link
Copy Markdown
Contributor Author

@vicb Would you mind re-reviewing this?

vicb
vicb approved these changes Oct 10, 2024
View reviewed changes
Comment thread test/workerd/tests.mjs Outdated
@anonrig
anonrig merged commit b1eed1a into main Oct 10, 2024
@anonrig
anonrig deleted the yagiz/update-buffer-cloudflare branch October 10, 2024 22:49
vicb added a commit that referenced this pull request Oct 11, 2024
@vicb
Revert "refactor(node/buffer): update cloudflare hybrid exports (#321)"
bd83310 
This reverts commit b1eed1a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pi0 pi0 pi0 approved these changes
@vicb vicb vicb approved these changes
@jasnell jasnell Awaiting requested review from jasnell
@petebacondarwin petebacondarwin Awaiting requested review from petebacondarwin

Assignees

No one assigned

Labels

cloudflare

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anonrig @pi0 @vicb @petebacondarwin