Skip to content

fix(node): never use relative import for node packages#352

Closed
vicb wants to merge 2 commits into
mainfrom
node-import
Closed

fix(node): never use relative import for node packages#352
vicb wants to merge 2 commits into
mainfrom
node-import

Conversation

@vicb

@vicb vicb commented Nov 29, 2024

Copy link
Copy Markdown
Contributor

We should never import Nodejs packages with relative import as unenv will not get a chance to replace those.

Relates to opennextjs/opennextjs-cloudflare#147: It looks like the issue is caused by the unenv implementation of Readable#[Symbol.AsyncIterator]:

  async *[Symbol.asyncIterator](): NodeJS.AsyncIterator<any> {
    throw createNotImplementedError("Readable.asyncIterator");
  }

However this should never be used by workerd that has builtin stream.

The second commits add a dummy IncomingMessage#_read to make testing easier.

@vicb vicb requested review from a team and pi0 as code owners November 29, 2024 13:37
@pi0

pi0 commented Nov 29, 2024

Copy link
Copy Markdown
Member

This breaks the direct fetch / http call mechanism of nitro, h3, and unenv itself in the Node.js environment (see this for ref), the reason is that Node.js polyfills of node:http in unenv depend on other node: polyfills (such as node:net) which are compatible with each other changed in this PR.

I think we need to find a different way to solve worked situation (simplest way I would imagine is to add any missing API to polyfills) and if you can kindly please open an issue with reproduction first I would be happy to help but so sorry this is really not a possible solution.

@pi0 pi0 closed this Nov 29, 2024
@pi0

pi0 commented Nov 29, 2024

Copy link
Copy Markdown
Member

(BTW generally we migrated and should change all other imports to node:, I noticed few node:events were relative we should fix only http/net/streams are that currently are functional with each other)

@vicb vicb changed the title node-import fix(node): never use relative import for node packages Nov 29, 2024
@pi0 pi0 deleted the node-import branch February 7, 2025 11:47
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