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

web-streams-polyfill/ponyfill interferes with another package #555

Closed
btmnk opened this issue Feb 19, 2023 · 8 comments · Fixed by #558
Closed

web-streams-polyfill/ponyfill interferes with another package #555

btmnk opened this issue Feb 19, 2023 · 8 comments · Fixed by #558
Assignees

Comments

@btmnk
Copy link

btmnk commented Feb 19, 2023

Hello,

I noticed since I updated nats.js that some parts of my applications don't work anymore.
Specifically when using discord.js which uses undici under the hood for sending stuff to a channel.

The web-streams-polyfill package was introduced in the 2.8.0 release of nats.

/node_modules/.pnpm/web-streams-polyfill@3.2.1/node_modules/web-streams-polyfill/dist/ponyfill.js:362
  throw new TypeError(context + " is not a ReadableStream.");
            ^

  TypeError: First parameter has member 'readable' that is not a ReadableStream.
    at assertReadableStream (/node_modules/.pnpm/web-streams-polyfill@3.2.1/node_modules/web-streams-polyfill/dist/ponyfill.js:362:19)
    at convertReadableWritablePair (/node_modules/.pnpm/web-streams-polyfill@3.2.1/node_modules/web-streams-polyfill/dist/ponyfill.js:3524:9)
    at ReadableStream.pipeThrough (/node_modules/.pnpm/web-streams-polyfill@3.2.1/node_modules/web-streams-polyfill/dist/ponyfill.js:3608:29)
    at fetchFinale (/node_modules/.pnpm/undici@5.15.0/node_modules/undici/lib/fetch/index.js:977:52)
    at mainFetch (/node_modules/.pnpm/undici@5.15.0/node_modules/undici/lib/fetch/index.js:777:5)

Is there any way to prevent Nats from using this package? I tested it by removing these lines:

if (typeof globalThis.ReadableStream === "undefined") {
    const streams = require("web-streams-polyfill/ponyfill");
    global.ReadableStream = streams.ReadableStream;
}

And it worked fine for my use case but I'm not sure how this could negatively impact the usage of nats overall.. I also understand that it would probably be no option to find a way to remove it from the package.

Have there been similar issues and is there a workaround known?
It seems I can only revert back to 2.6.1 for the time being..

@aricart
Copy link
Member

aricart commented Feb 21, 2023

@btmnk wondering if you require nats after you require your other component whether it works (the object store functionality will not in nats) - the other library is performing a similar patch with something else because this is why it is crashing on the other side.

@aricart
Copy link
Member

aricart commented Feb 21, 2023

@btmnk are you in node 16 or better? If you are in node 14, the only recourse is to disable it because the stream/web is not available there.

I am wondering if you perform these changes, whether your environment works - if it is, I am happy to just clamp it to stream/web which should be what is used by fetch and not support node 14 since it is about to fall of the support bus in April.

To test:

Instead of deleting the above, change it to:

if (typeof globalThis.ReadableStream === "undefined") {
  // @ts-ignore: node global
  const chunks = process.versions.node.split(".");
  const v = parseInt(chunks[0]);
  console.log(v);
  const lib = v >= 16 ? "stream/web" : "web-streams-polyfill/ponyfill";
  const streams = require(lib);
  global.ReadableStream = streams.ReadableStream;
}

@aricart
Copy link
Member

aricart commented Feb 21, 2023

Alternatively if you run in node 19, the patching shouldn't happen as it is part of the global object in node.

@aricart
Copy link
Member

aricart commented Feb 21, 2023

@btmnk I was able to reproduce your problem:

import {connect, StringCodec} from "nats";
import {fetch, Agent} from "undici";

const res = await fetch('http://localhost:8080/varz', {
  dispatcher: new Agent({
    keepAliveTimeout: 10,
    keepAliveMaxTimeout: 10
  })
})
const json = await res.json()
console.log(json)

const nc = await connect();
const js = nc.jetstream();

const os = await js.views.os("hello");

function readableStreamFrom(data) {
    return new ReadableStream({
      pull(controller) {
        controller.enqueue(data);
        controller.close();
      },
    });
  }

const sc = StringCodec();
const v = await os.put({ name: "a" }, readableStreamFrom(sc.encode("hello")));
console.log(v);

And with the above patch both nats and undici seem to coexist fine.

/p/t/test [1]$ node index.js
{
  server_id: 'NBIPKITOZ7IIBIHSJ3RLJ5YDHKMYV4N6HDXUO5UCRQE66KBPG7N7PPLU',
  server_name: 'NBIPKITOZ7IIBIHSJ3RLJ5YDHKMYV4N6HDXUO5UCRQE66KBPG7N7PPLU',
  version: '2.10.0-beta.16',
  proto: 1,
  go: 'go1.19.3',
  host: '0.0.0.0',
  port: 4222,
  max_connections: 65536,
  ping_interval: 120000000000,
  ping_max: 2,
  http_host: '0.0.0.0',
  http_port: 8080,
  http_base_path: '',
  https_port: 0,
  auth_timeout: 2,
  max_control_line: 4096,
  max_payload: 1048576,
  max_pending: 67108864,
  cluster: {},
  gateway: {},
  leaf: {},
  mqtt: {},
  websocket: {},
  jetstream: {
    config: {
      max_memory: 51539607552,
      max_storage: 411210753024,
      store_dir: '/tmp/js/jetstream'
    },
    stats: {
      memory: 0,
      storage: 0,
      reserved_memory: 0,
      reserved_storage: 0,
      accounts: 1,
      ha_assets: 0,
      api: [Object]
    }
  },
  tls_timeout: 2,
  write_deadline: 10000000000,
  start: '2023-02-21T13:23:25.163509Z',
  now: '2023-02-21T13:26:15.876986Z',
  uptime: '2m50s',
  mem: 8822784,
  cores: 16,
  gomaxprocs: 16,
  cpu: 0,
  connections: 0,
  total_connections: 2,
  routes: 0,
  remotes: 0,
  leafnodes: 0,
  in_msgs: 0,
  out_msgs: 0,
  in_bytes: 0,
  out_bytes: 0,
  slow_consumers: 0,
  subscriptions: 54,
  http_req_stats: { '/varz': 5 },
  config_load_time: '2023-02-21T13:23:25.163509Z',
  system_account: '$SYS',
  server_key: 'NBIPKITOZ7IIBIHSJ3RLJ5YDHKMYV4N6HDXUO5UCRQE66KBPG7N7PPLU'
}
 >> jetstream's materialized views object store functionality in nats.js is beta functionality 
ObjectInfoImpl {
  info: {
    bucket: 'hello',
    nuid: 'Y4YHMFM934NYFECQUEPGAP',
    size: 5,
    chunks: 1,
    name: 'a',
    description: '',
    options: { max_chunk_size: 131072 },
    mtime: '2023-02-21T13:26:15.905Z',
    digest: 'SHA-256=LPJNul-wow4m6DsqxbninhsWHlwfp0JecwQzYpOLmCQ=',
    deleted: false
  }
}

@aricart
Copy link
Member

aricart commented Feb 21, 2023

I will do a release today

@btmnk
Copy link
Author

btmnk commented Feb 21, 2023

Thank you so much!
I didn't expect a solution so fast..
You had the solution even before I saw the first comment xD

I also tested your patch in my environment and it works fine!
(All my containers are on node 16 right now)

@btmnk
Copy link
Author

btmnk commented Feb 21, 2023

I also tried just using node 19 and that worked like a charm as well.
I probably will stay on node 19 then since the rest of the application works fine with it so far.
(I didn't know it was already the current release)

Thank you so much for the tips!

aricart added a commit that referenced this issue Feb 21, 2023
…into node 16+) - this is required to support ReadableStream in ObjectStore and not create inconsistencies with fetch and other libraries. FIX #555
aricart added a commit that referenced this issue Feb 21, 2023
…into node 16+) - this is required to support ReadableStream in ObjectStore and not create inconsistencies with fetch and other libraries. FIX #555 (#558)
@aricart
Copy link
Member

aricart commented Feb 21, 2023

npm install nats@latest

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 a pull request may close this issue.

2 participants