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

fix(ext/node): use primordials in ext/node/polyfills/internal/buffer.mjs #24993

Merged

Conversation

petamoriken
Copy link
Contributor

Towards #24236

@bartlomieju
Copy link
Member

We effectively vendor this file. I'm not sure if it's worth using primordials here. Feels like it's gonna make updates a lot harder. That said we only update it a couple times per year, so maybe it's not such a big deal?

@petamoriken
Copy link
Contributor Author

IMHO, primordials are hard to introduce, but then updating the codes is not so hard.

@petamoriken
Copy link
Contributor Author

petamoriken commented Aug 12, 2024

Since Node.js also uses primordials, the diffs with the Deno impl may decrease and make it rather easy to update.
https://github.com/nodejs/node/blob/0301309493b2a4e80796517bdb46ec55202a4800/lib/internal/buffer.js

@bartlomieju
Copy link
Member

@petamoriken, sorry for a slow response. I was wrong, it's not this file that we vendor so this should be fine. Can you please rebase the PR?

@petamoriken petamoriken force-pushed the fix/primordials-node-internal-buffer branch from 2eb4059 to 050d09d Compare October 15, 2024 00:30
@bartlomieju bartlomieju added this to the 2.1.0 milestone Oct 15, 2024
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@bartlomieju bartlomieju merged commit 7211028 into denoland:main Oct 15, 2024
17 checks passed
@petamoriken petamoriken deleted the fix/primordials-node-internal-buffer branch October 15, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants