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

chore: add node specifier to assert import #856

Open
wants to merge 1 commit into
base: v4
Choose a base branch
from

Conversation

Hebilicious
Copy link

Description

Hello there,
In order to use this library in some runtimes (ie cloudflare workers), I have to patch this library with an import specifier. I believe this change could benefit other users.

Additionally, I haven't looked at the rest of the codebase, so maybe you would want to add node specifiers everywhere in one single PR ?

Performance impact

Security impact

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

Hello there,
In order to use this library in some runtimes (ie cloudflare workers), I have to patch this library with an import specifier. 
I believe this change could benefit other users.

Additionally, I haven't looked at the rest of the codebase, so maybe you would want to add node specifiers everywhere in one single PR ?
@benjie
Copy link
Member

benjie commented Mar 13, 2024

If you’re doing it, it needs doing via a lint rule to enforce it in future imports too. However I have a vague recollection that adding a specifier like this actually breaks it on some runtimes (bun?), have a look through the history to see if specifiers were removed at one point.

@mderrick
Copy link

I tried patching this myself for Cloudflare workers but am met with Uncaught Error: Dynamic require of "node:assert" is not supported and had to remove assert entirely and replace it with a manual throw. It would be great if this package could support CF workers.

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.

3 participants