Add browser entrypoint #124
Conversation
| @@ -13,6 +13,7 @@ | |||
| "type": "module", | |||
| "exports": { | |||
| "types": "./source/index.d.ts", | |||
| "browser": "./source/exports.js", | |||
| "default": "./source/index.js" | |||
There was a problem hiding this comment.
There is also the browser top-level field, which is older and predates exports.browser. I am not sure which style do you prefer, and whether I am missing some other field?
There was a problem hiding this comment.
No, this is all we need. The browser field is underspecified and legacy.
| @@ -142,6 +142,16 @@ try { | |||
| } | |||
| ``` | |||
|
|
|||
| ## Browser support | |||
There was a problem hiding this comment.
Documenting it to be pro-active since users are most likely going to open issues with some unique build configuration where this might not work. :)
| @@ -1,7 +1,7 @@ | |||
| import {isReadableStream} from 'is-stream'; | |||
|
|
|||
| export const getAsyncIterable = stream => { | |||
| if (isReadableStream(stream, {checkOpen: false})) { | |||
| if (isReadableStream(stream, {checkOpen: false}) && nodeImports.on !== undefined) { | |||
There was a problem hiding this comment.
In principle, browsers do not support Node streams, so if stream is a Node Readable, then nodeImports.on should be defined.
However, there might be some exotic environment which would load the browser entrypoint and have Node streams support. For example, maybe Deno.
So I added an extra check there.
ehmicky
force-pushed
the
node-static-imports
branch
from
7540a6a to
7079dfc
Compare
| const error = new Error('test'); | ||
| stream.emit('error', error); | ||
| t.is(await t.throwsAsync(promise), error); | ||
| }); |
There was a problem hiding this comment.
The above was not working when using dynamic imports, since the error listeners would be set too late. This test makes sure this now works.
| import {finished} from 'node:stream/promises'; | ||
| import {nodeImports} from './stream.js'; | ||
|
|
||
| Object.assign(nodeImports, {on, finished}); |
There was a problem hiding this comment.
An alternative would be to bind those with each exported method.
However, using a top-level variable results in simpler code, since those "Node imports" do not need to be passed around through the codebase.
ehmicky
force-pushed
the
node-static-imports
branch
from
7079dfc to
e4c1bd1
Compare
|
Merge conflict fixed. 👍 Also, I don't have any other pending PR for this repo, so we're ready for a major release after it. |
Fixes #123.