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

Ready to boost your popularity to like 22 mil download / week? #82

Closed
jimmywarting opened this issue Jul 20, 2021 · 14 comments · Fixed by #83
Closed

Ready to boost your popularity to like 22 mil download / week? #82

jimmywarting opened this issue Jul 20, 2021 · 14 comments · Fixed by #83
Milestone

Comments

@jimmywarting
Copy link

jimmywarting commented Jul 20, 2021

Do you think you can reduce the bundle by a magnitude?
node-fetch/node-fetch#1217

We have always been hesitated to include this package cuz of its size... We also would never thought NodeJS would ship whatwg streams that it would become useful. but it looks like we are in a area where ppl demand more spec'ed cross compatible code across Deno, NodeJS and Browsers instead

There is a ambition to get whatwg streams into node-fetch body as well. (b/c NodeJS started to support whatwg streams) (planed for v4)

@MattiasBuelens
Copy link
Owner

Sooooo yeah, my initial intention for this polyfill was to target web browsers. I noticed that it also runs fine on Node, so I slapped a main field in package.json pointing at an UMD bundle and called it a day. 😛

I haven't put much thought into optimizing for download size. I provide (too) many variants in a single npm package, so that you can install one and then import whichever variant you're interested in (e.g. import from "web-streams-polyfill/es2018"). Users can even load most variants directly as a <script> tag.

I could overhaul the project and remove all of the transpilation, bundling and minification, and instead compile every TypeScript source file to an individual JavaScript file (basically tsc -p .). That would solve the problem for Node users, but it would make life much more difficult for web developers building for (older) browsers.

Alternatively, I could publish it as a separate package aimed for Node users. So you'd have web-streams-polyfill for web developers with all of its variants, and then node-web-streams-polyfill (or something) for Node users. I'd probably need to update the README to explain when to use which. 😛

Looking further ahead, maybe it would make sense to have a dedicated, separate polyfill for Node? Instead of tracking the reference implementation, it could track the Node implementation instead. It could also polyfill the adapters between WHATWG streams an Node streams that were added in nodejs/node#39134. 🤔

@jimmywarting
Copy link
Author

jimmywarting commented Jul 20, 2021

I would rather restrict the polyfill to only accept streams from its own version, rather than trying to make multiple versions compatible with each other. For example, readableFromVersion1.pipeTo(writableFromVersion2) should always immediately reject with a TypeError here, indicating that the given writable is not valid.

This had me concerned... what if two packages use different version of the stream polyfill or one includes the cjs and another includes the esm? node-fetch uses one version and the developer who use node-fetch uses something else...? and one other use require('stream').web

usually when you do something on your own internally then it's fine to use the same version but when you expose public api's that uses whatwg streams between different packages then it becomes a problem with interchangeable streams

I provide (too) many variants in a single npm package

yea, that seems to be the issue here

Looking further ahead, maybe it would make sense to have a dedicated, separate polyfill for Node? Instead of tracking the reference implementation, it could track the Node implementation instead. It could also polyfill the adapters between WHATWG streams an Node streams that were added in nodejs/node#39134.

I would want to see the implementation of ReadableStream.from(iterable_stream) proposal

@MattiasBuelens
Copy link
Owner

This had me concerned... what if two packages use different version of the stream polyfill or one includes the cjs and another includes the esm? node-fetch uses one version and the developer who use node-fetch uses something else...?

For simplicity, we currently require all streams to be from the exact same version of the polyfill. If we want to allow streams from "foreign" polyfill versions, it has to be a conscious decision and we have to be very clear what is allowed and what isn't. I'm not opposed to the idea, but it's gonna be more work to get right.

For example: with cases like #75, rs.pipeTo(ws) should ideally accept any stream that implements the public WritableStream API. That is: it has a getWriter() method which returns a writer with write(), close() and abort() methods. We'd have to rework our pipeTo() implementation to not rely on any internal fields (like _writableStreamController), and I'm not 100% sure if it's possible to achieve perfect spec compliance that way.

For CJS/ESM, we could use the ES module wrapper approach for dual packages. But yeah, that doesn't really help for different versions installed in different parts of the dependency tree.

I would want to see the implementation of ReadableStream.from(iterable_node_stream) proposal

That would be the nicer solution. However, the behavior of nodeReadable.toWeb() is not 100% identical to ReadableStream.from(nodeReadable) because the intermediate async iterator hides some details (e.g. you can't actually cancel a pending read with it.return(), while you can with reader.cancel()).

Also, I don't know if the Streams standard will ever have WritableStream.from() or TransformStream.from(), so there wouldn't be a counterpart for nodeWritable.toWeb() or nodeDuplex.toWeb(). 😕

@jimmywarting
Copy link
Author

jimmywarting commented Jul 20, 2021

For CJS/ESM, we could use the ES module wrapper approach for dual packages. But yeah, that doesn't really help for different versions installed in different parts of the dependency tree.

I think wrapper approach could solve the same instanceof part if you just use a ES module wrapper, but it would require you to switch all import to require everywhere else... (and only use import in the wrappers files) seems like a unappealing approach but something that could work, really the only thing that changes about a wrapper approach is how you load the files.
either way i don't think the issue is really about cjs vs esm (wrapper). i just wanted to get to the point of where the current bundle provides (too) many different variants/instances. the current bundle duplicates all the code instead of using a wrapper

it would seem like the best option is to provide one variant that is compiled to old syntax (combined with ES wrapper) even if it means that a file becomes larger. but at least you will get rid of more foreign streams.

the bad part about wrappers is that they don't really work in browser cuz browser don't support commonjs


Cool, now i learned that there is a easy way to convert node streams with nodeReadable.toWeb()

@jimmywarting
Copy link
Author

jimmywarting commented Jul 20, 2021

Could you provide a semi public/private method/backdoor into the streams internals that allows you to cancel the internal read between two different versions or something like that? 🤔

@MattiasBuelens
Copy link
Owner

I think wrapper approach could solve the same instanceof part if you just use a ES module wrapper, but it would require you to switch all import to require everywhere else... (and only use import in the wrappers files) seems like a unappealing approach but something that could work, really the only thing that changes about a wrapper approach is how you load the files.

I would configure TypeScript with module: 'commonjs' so it can compile all import statements to require calls. Then I would write an ESM wrapper that requires the (compiled) CommonJS entry point.

either way i don't think the issue is really about cjs vs esm (wrapper). i just wanted to get to the point of where the current bundle provides (too) many different variants/instances. the current bundle duplicates all the code instead of using a wrapper

I agree, there's just too many of them... I can probably remove and/or merge a few without too much worries, but I don't know if it'll be enough. 😕

Cool, now i learned that there is a easy way to convert node streams with nodeReadable.toWeb()

Yup! Very cool stuff. 😁

Could you provide a semi public/private method/backdoor into the streams internals that allows you to cancel the internal read between two different versions or something like that? 🤔

That just means that the stream internals also become part of the public API... 😛 It would make backward and forward compatibility more difficult.

@jimmywarting
Copy link
Author

jimmywarting commented Jul 20, 2021

I agree, there's just too many of them... I can probably remove and/or merge a few without too much worries, but I don't know if it'll be enough. 😕

That would probably be best.

hope you didn't miss my update about browser not being able to unwrap esm wrappers... <script type="module" src="wrapper.esm">

FYI. https://jspm.dev/ is a awesome CDN tools that allows you to load any npm/cjs module in Deno/browser without much hassle. clearly more superior than jsdeliver or cdnjs or any other. it converts the file for you to ESM

That just means that the stream internals also become part of the public API... 😛 It would make backward and forward compatibility more difficult.

it would be a undocumented public api starting with _ that is only meant for the polyfill 😛
it would probably just call some undocumented function that dispatch a event (without using EventEmitter or EventTarget)

I just learned a cool hack from NodeJS that goes something like this:

class Headers {
  #headers = []
  
  static getRawHeaders(headers) {
    const result = {};
    for (const { 0: key, 1: values } of headers.#headers.entries()) {
      ...
    }
    return result;
  }
}

// backdoor
const private_method_shared_internally_in_core = getRawHeaders
delete Headers.getRawHeaders

@jimmywarting
Copy link
Author

jimmywarting commented Jul 21, 2021

...Track the Node implementation instead

Speaking of tracking NodeJS implementation
shouldn't readable-stream implement nodejs references so that you can do import('node:stream/web')? and have it well browserified? then we could include readable-stream in older nodejs versions.

Maybe that would be a good place to put it...?
but it would also be annoying to have to install node:streams, legacy string-decoder, inherit, util-depricate, and safe-buffer + node:buffer that has nothing to do with whatwg:streams
image
perhaps node:streams should just use a dependency on your impl

@jimmywarting
Copy link
Author

jimmywarting commented Jul 21, 2021

What is your thought process on ditching cjs entirely? Many developers have already ditched cjs and switched to <script type="module"> and stop supporting IE11. ppl are getting tiered of having to convert/support two kinds of modules.

...we decided to ditch cjs in node-fetch@3, fetch-blob@3, formdata-polyfill, native-file-system-adapter and put the lowest targeted node support to 12.20 in package.json. and smashed in type=module

sindresorhus is converting all of his 1k packages to esm sindresorhus/meta#15

webtorrent and all sub packages is about to as soon as the test framework they use gets unblocked and starts to support esm

@MattiasBuelens
Copy link
Owner

What is your thought process on ditching cjs entirely? Many developers have already ditched cjs and switched to <script type="module"> and stop supporting IE11. ppl are getting tiered of having to convert/support two kinds of modules.

That's also a possibility. I'm thinking:

  • Add type: "module" to package.json, and make sure to use import in the test code as well (e.g. run-web-platform-tests.js).
  • Build dist/ponyfill.js as an ESM bundle with ES2015 syntax (equivalent to the current dist/ponyfill.es6.mjs)
  • Compile src/polyfill.ts directly to dist/polyfill.js without bundling. That way, it'll import the implementation from dist/ponyfill.js instead of duplicating it again. I'll see if I can figure something out with Rollup's code splitting. 🤔
  • Remove the UMD bundles. Tell users to either configure their bundlers to consume and transform the ESM bundle, or to use a service like jspm.io/jspm.dev.

This will be quite the breaking change. I should probably start a next branch. 😛

@jimmywarting
Copy link
Author

Feel free to close this if you want.

Would love to see some new (meta) issue, milestone or just a draft PR about what is coming next in v4 so i can stay in the loop and what's being worked on :)

@MattiasBuelens
Copy link
Owner

Not sure yet. I would like to (finally) tackle #20, but I'm afraid it might take too long and people might get annoyed about being stuck on v4.0.0-beta.1 for ages. 😛 So I might just release v4.0.0 as-is (save for a few fixes) and leave #20 for a v5.0.0. 🤷

For now, there's just the v4.0.0 milestone. If I have a more concrete plan, I'll make sure to put a new issue under that milestone. 😉

@jimmywarting
Copy link
Author

woho! Have long been waiting for #20 🙂

@MattiasBuelens
Copy link
Owner

@jimmywarting You probably already moved on from web-streams-polyfill to Node.js's native implementation. But on the off chance that you'd still need a full-blown polyfill: version 4.0.0 is out, and it weighs just 441 KB (with each variant weighing around 60-70 kB). 🎉

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