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

Implement Blob.stream, Blob.text and Blob.arrayBuffer #2555

Open
jimmywarting opened this issue Apr 15, 2019 · 34 comments
Open

Implement Blob.stream, Blob.text and Blob.arrayBuffer #2555

jimmywarting opened this issue Apr 15, 2019 · 34 comments

Comments

@jimmywarting
Copy link

jimmywarting commented Apr 15, 2019

We over at node-fetch has a response.blob() method that allows you to get a blob like object, it's currently useless cuz you can't read it in any way except from doing:

new Response(blob).arrayBuffer()

We are planing on adding Blob.text() and Blob.arrayBuffer() at least, not sure about blob.stream() yet if it's going to be a node stream or a web stream
we do not currently expose the constructor in any way but we would like to make this compatible with your FileReader. If you changed the FileReader api to read the content of the file using this methods you could read any blob/file like object not only coming from your own jsdom implementation.

fr.readAsBuffer() --> blob.arrayBuffer().then(dispatch)
fr.readAsText() --> blob.text().then(dispatch)
fr.readAsDataURL() --> blob.arrayBuffer().then(buffer2dataURL).then(dispatch)

@domenic
Copy link
Member

domenic commented Apr 15, 2019

I'm not sure what the exact ask is here, but I'll note that jsdom, like browsers, reads the contents of things using internal APIs, so that you can't fool it with "non web platform" objects (i.e. non-jsdom objects). This is by design.

@jimmywarting
Copy link
Author

There is now new ways to to read blob/files using a promise based api that exist as a method on the Blob class, mainly Blob.prototype.text and Blob.prototype.arrayBuffer when called both returns a promise. this is what i want you to implement

@jimmywarting
Copy link
Author

jimmywarting commented Apr 15, 2019

What I'm also asking for is:
That you change the way the FileReader how it reads the content. Using the new "internal public APIs" on the blob itself instead of reading it from blob._buffer

@domenic
Copy link
Member

domenic commented Apr 15, 2019

These are not internal APIs; they are public APIs. So this will not generally work, just like what you're asking for won't work in browsers.

@jimmywarting
Copy link
Author

ups, meant public apis

@domenic
Copy link
Member

domenic commented Apr 15, 2019

Yeah, that would be going against the spec, so it's not an option for us.

@jimmywarting
Copy link
Author

Blob.stream, Blob.text and Blob.arrayBuffer are speced, not just implemented anywhere yet

@domenic
Copy link
Member

domenic commented Apr 15, 2019

Right, but if you override them to throw an exception, then FileReader will still work (per spec), because FileReader uses internal data access, not the public APIs.

Your request is that if you override them to throw an exception, FileReader will call those public APIs and fail. We are not interested in violating the spec in that way.

@jimmywarting
Copy link
Author

jimmywarting commented Apr 15, 2019

okey, so apart from my second request about changing the FileReader to use the public blob api. Would you consider adding the new public blob methods that allows you to get things with a promise?

@domenic
Copy link
Member

domenic commented Apr 15, 2019

For sure. Pull request welcome!

@jimmywarting
Copy link
Author

jimmywarting commented Apr 15, 2019

Actually, when i read the readAsText spec now it says

  1. If fr ’s state is "loading" , throw an InvalidStateError DOMException .
  2. Set fr ’s state to "loading" .
  3. Set fr ’s result to null .
  4. Set fr ’s error to null .
  5. Let stream be the result of calling get stream on blob .
  6. Let reader be the result of getting a reader from stream .
  7. ...
  8. ...

so it states that the FileReader should read the content of the blob using the Public API's
not some internal things?

@domenic
Copy link
Member

domenic commented Apr 15, 2019

@jimmywarting
Copy link
Author

jimmywarting commented Apr 15, 2019

thought https://pr-preview.s3.amazonaws.com/w3c/FileAPI/117/795750f...42f8a45.html#blob-get-stream was the same thing as calling https://pr-preview.s3.amazonaws.com/w3c/FileAPI/117/795750f...42f8a45.html#stream-method-algo

That the "A Blob blob has an associated get stream algorithm" which was the same thing as calling blob.stream()

@domenic
Copy link
Member

domenic commented Apr 15, 2019

Nope, #blob-get-stream is private, which two public methods stream() and readAsText() both call. Pretty standard pattern for factoring out "private methods" in spec text.

@jimmywarting
Copy link
Author

jimmywarting commented Apr 15, 2019

oh well, still think you should use the public method to read the blob... would help some folks that use node-fetch that calls response.blob() to get a jsdom-like object - not sure why they do it if they are going to read it anyway with with fr.readAsWhatever(), could as well call response.arrayBuffer() directly or something like that

but if you are so strict, so be it.
I'm not using jsdom or any combination of both node-fetch + jsdom. So it's not my problem

@Gozala
Copy link

Gozala commented Apr 15, 2019

Any chances Blob implementation could be extracted into separate library so both libraries could be made interoperable ? Right now both libraries have a strong opinions and as result they can't interop and combination of two is pretty much unusable.

@Gozala
Copy link

Gozala commented Apr 16, 2019

@domenic I think @jimmywarting is proposing very specific solution, although actual goal here is to make two libraries interoperable. Maybe we can shift discussion towards how two libraries can interop ?

Specific issue I've being running into in the context of WebMemex/freeze-dry#44 is that project uses node-fetch and jsdom in nodejs where things seems to misbehave because blob instanceof Blob is false when one comes from jsdom and other from node-fetch because each bundles own implementation of Blob.

I believe what @jimmywarting was hoping is for both libraries to migrate from checks like blob instanceof Blob to blob.arrayBuffer(), which would resolve incompatibility issues.

@domenic would you please collaborate with us on a solution to this interop issues which would not require node-fetch to depend on jsdom ?

@Sebmaster
Copy link
Member

would you please collaborate with us on a solution to this interop issues which would not require node-fetch to depend on jsdom ?

The problem here is that you include node-fetch at all. If your scripts ran in jsdom, your isomorphic libs should act as if they're in a browser (that is: detect the presence of XmlHttpRequest or similar) and get the "browser's" (jsdom's) native Blob implementation.

@Gozala
Copy link

Gozala commented Apr 17, 2019

The problem here is that you include node-fetch at all. If your scripts ran in jsdom, your isomorphic libs should act as if they're in a browser (that is: detect the presence of XmlHttpRequest or similar) and get the "browser's" (jsdom's) native Blob implementation.

I am not sure how did you arrive to that conclusion, but I respectfully disagree. Use only JSDOM or no JSDOM at all isn’t a great solution for the community, there are & will continue to be use cases that don’t fit this binary proposition

P.S.: My scripts aren’t run by JSDOM, it is used mostly to represent document in DOM API compatible API.

@Sebmaster
Copy link
Member

Use only JSDOM or no JSDOM at all isn’t a great solution for the community

Not what I said.

there are & will continue to be use cases that don’t fit this binary proposition

Please do elaborate with specifics. It usually makes no sense to have a hybrid environment for testing, since that means you're not actually testing in any reasonably expected environment.

P.S.: My scripts aren’t run by JSDOM, it is used mostly to represent document in DOM API compatible API.

jest usually runs tests in jsdom by default unless you explicitly disable it. If you haven't configured that, the problem is in the detection of a "node environment" in the fetch implementation. It should detect that it's running in a browser.

If you did explicitly configure it otherwise, then you're mixing browser and non-browser environments which is something we explicitly recommend against (the pattern doesn't quite apply, but the results are the same) because of exactly these issues.

@domenic
Copy link
Member

domenic commented Apr 17, 2019

I agree overall with @Sebmaster that you should be running your scripts inside the jsdom, and that using node-fetch (instead of the whatwg-fetch polyfill) is probably the core of the problem.

Stated another way, jsdom emulates a web browser---and that's a large project, enough to keep us quite busy. It seems like some folks in this issue thread are asking us to expand the project's scope beyond "just" emulating a web browser, to also emulate Electron, i.e. have mechanisms for bridging between Node-level primitives and the "web browser environment" jsdom provides. That is, whatwg-fetch runs in a browser; node-fetch can only run in a browser if that browser also grows all the Node-level capabilities that Electron has.

That's not really feasible for the project as it is currently scoped or envisioned. It may be an interesting project for someone else to pursue on top of jsdom, similar to how Electron is built on top of Chromium.

@gap777
Copy link

gap777 commented Jun 16, 2021

Here's a simple scenario I think should be supported, but is not (due to the points made here, specifically by @jimmywarting and @Gozala): testing code that fetches an Image from a URL, uses FileReader to populate an Image, extracts width & height, and then proceeds to process the image data (save to S3, for instance).

  1. create a Blob via blob-utils dataURLToBlob and a small PNG data URI in my test file
  2. setup a response with fetch-mock for the given image URL to yield the Blob setup in step 1
  3. invoke my function under test

I was surprised to discover today the incompatibility between fetch-mock+node-fetch and jsdom in this area. I think that @Gozala has a great point-- let's find a way to make these two compatible.

Why is it so important for jsdom to use it's own Blob? Why couldn't it be extracted out into a separate package, usable by (in this case) both jsdom and node-fetch?

Why is it so important for jsdom's FileReader to access Blob via private variables _buffer instead of public interfaces?

The request here is not so unreasonable, and yet it seems like all effort of finding common ground has ceased. 2 ideas have already been put forth and rejected by one side... can the other side at least suggest some alternatives besides "you're doing it wrong"?

@cranesandcaff
Copy link

cranesandcaff commented Mar 16, 2022

https://developer.mozilla.org/en-US/docs/Web/API/Blob

Array buffer is implemented in everything but IE, could we get an update on this?

@domenic
Copy link
Member

domenic commented Mar 16, 2022

https://mobile.twitter.com/slicknet/status/782274190451671040

@tom-james-watson
Copy link

A workaround for this for jest is to add a setupFilesAfterEnv script that does a

require("blob-polyfill");

@nemanjam
Copy link

This works for File constructor.

@Timmmm
Copy link

Timmmm commented Apr 7, 2022

How do we actually access the contents of the Blob with JSDOM if none of these methods are implemented? Is it impossible?

@DGollings
Copy link

DGollings commented May 12, 2022

Didn't try @tom-james-watson's workaround (I should have and likely will)

But here's my, lets call it jsdom-native, workaround

const readFile = async (file: File) => {
  return new Promise<ArrayBuffer>((resolve, reject) => {
    const reader = new FileReader();
    reader.onload = function () {
      return resolve(reader.result as ArrayBuffer);
    };
    reader.onerror = function () {
      return reject(reader.error);
    };
    reader.readAsArrayBuffer(file);
  });
};

There's a good chance require("blob-polyfill"); does almost the exact same, but written by someone competent in the matter :)

EDIT: require("blob-polyfill"); works perfectly

@jimmywarting
Copy link
Author

jimmywarting commented Aug 9, 2022

fyi, i still think you should be using the public api:s that exist on the blob to read the data from those instead in the FileReader

NodeJS v18 now has spec compatible FormData, Blob, File, fetch, Request, Response, Headers, whatwg streams and all of the things.
I think you should use them instead whenever it's available and fallback to some polyfill if needed

NodeJS blob are more spec compliant than your own blob impl that still lacks stream() arraybuffer() and text()
and implementing .stream() and whatwg stream is a lot of undertaking and bringing in web-stram-polyfills adds a lot of other complications too


to get a hold of the File class provided by NodeJS you would have to do:

const fd = new FormData()
fd.set('x', new Blob())
const File = fd.get('x').constructor

Right, but if you override them to throw an exception, then FileReader will still work (per spec), because FileReader uses internal data access, not the public APIs.

Your request is that if you override them to throw an exception, FileReader will call those public APIs and fail. We are not interested in violating the spec in that way.

I would not worry about someone patching Blob.prototype to do anything else badly...
if you are then maybe you could do:

// cache the method
const Blob = globalThis.Blob
const read = Blob.prototype.arrayBuffer

FileReader.prototype.readAsArrayBuffer = function(blob) {
  read.call(blob).then(...)
}

@exige81
Copy link

exige81 commented Oct 4, 2022

I was going to work on a PR for Blob.arrayBuffer() but looks like it will be a no-go for now as the wpt relies on TextEncoder which is not supported at this time ref: #2524.

@jimmywarting
Copy link
Author

Given now that we also got fs.openAsBlob(path) now also then i think there is even bigger reason to still use public api's in the FileReader so those can be read also.

I think you should be replacing your blob impl with built in NodeJS blobs

mistic added a commit to elastic/kibana that referenced this issue Jul 20, 2023
This PR adds the `Blob` polyfill into the jest jsdom env which is
currently not supported as mentioned in
jsdom/jsdom#2555

---------

Co-authored-by: Thomas Watson <w@tson.dk>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Jul 20, 2023
This PR adds the `Blob` polyfill into the jest jsdom env which is
currently not supported as mentioned in
jsdom/jsdom#2555

---------

Co-authored-by: Thomas Watson <w@tson.dk>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit e9a6519)
mistic added a commit to mistic/kibana that referenced this issue Jul 20, 2023
This PR adds the `Blob` polyfill into the jest jsdom env which is
currently not supported as mentioned in
jsdom/jsdom#2555

---------

Co-authored-by: Thomas Watson <w@tson.dk>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit e9a6519)

# Conflicts:
#	package.json
#	packages/kbn-test/src/jest/setup/polyfills.jsdom.js
#	yarn.lock
kibanamachine added a commit to elastic/kibana that referenced this issue Jul 20, 2023
# Backport

This will backport the following commits from `main` to `8.9`:
- [chore(NA): add Blob polyfill on jest env
(#162197)](#162197)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Tiago
Costa","email":"tiago.costa@elastic.co"},"sourceCommit":{"committedDate":"2023-07-20T17:30:43Z","message":"chore(NA):
add Blob polyfill on jest env (#162197)\n\nThis PR adds the `Blob`
polyfill into the jest jsdom env which is\r\ncurrently not supported as
mentioned
in\r\nhttps://github.com/jsdom/jsdom/issues/2555\r\n\r\n---------\r\n\r\nCo-authored-by:
Thomas Watson <w@tson.dk>\r\nCo-authored-by: kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"e9a651933768353e65c2c4e52b98b768bb7f3b27","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["chore","Team:Operations","release_note:skip","backport:all-open","v8.10.0"],"number":162197,"url":"#162197:
add Blob polyfill on jest env (#162197)\n\nThis PR adds the `Blob`
polyfill into the jest jsdom env which is\r\ncurrently not supported as
mentioned
in\r\nhttps://github.com/jsdom/jsdom/issues/2555\r\n\r\n---------\r\n\r\nCo-authored-by:
Thomas Watson <w@tson.dk>\r\nCo-authored-by: kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"e9a651933768353e65c2c4e52b98b768bb7f3b27"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"#162197:
add Blob polyfill on jest env (#162197)\n\nThis PR adds the `Blob`
polyfill into the jest jsdom env which is\r\ncurrently not supported as
mentioned
in\r\nhttps://github.com/jsdom/jsdom/issues/2555\r\n\r\n---------\r\n\r\nCo-authored-by:
Thomas Watson <w@tson.dk>\r\nCo-authored-by: kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"e9a651933768353e65c2c4e52b98b768bb7f3b27"}}]}]
BACKPORT-->

Co-authored-by: Tiago Costa <tiago.costa@elastic.co>
mistic added a commit to elastic/kibana that referenced this issue Jul 21, 2023
# Backport

This will backport the following commits from `main` to `7.17`:
- [chore(NA): add Blob polyfill on jest env
(#162197)](#162197)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Tiago
Costa","email":"tiago.costa@elastic.co"},"sourceCommit":{"committedDate":"2023-07-20T17:30:43Z","message":"chore(NA):
add Blob polyfill on jest env (#162197)\n\nThis PR adds the `Blob`
polyfill into the jest jsdom env which is\r\ncurrently not supported as
mentioned
in\r\nhttps://github.com/jsdom/jsdom/issues/2555\r\n\r\n---------\r\n\r\nCo-authored-by:
Thomas Watson <w@tson.dk>\r\nCo-authored-by: kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"e9a651933768353e65c2c4e52b98b768bb7f3b27","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["chore","Team:Operations","release_note:skip","backport:all-open","v8.10.0"],"number":162197,"url":"#162197:
add Blob polyfill on jest env (#162197)\n\nThis PR adds the `Blob`
polyfill into the jest jsdom env which is\r\ncurrently not supported as
mentioned
in\r\nhttps://github.com/jsdom/jsdom/issues/2555\r\n\r\n---------\r\n\r\nCo-authored-by:
Thomas Watson <w@tson.dk>\r\nCo-authored-by: kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"e9a651933768353e65c2c4e52b98b768bb7f3b27"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"#162197:
add Blob polyfill on jest env (#162197)\n\nThis PR adds the `Blob`
polyfill into the jest jsdom env which is\r\ncurrently not supported as
mentioned
in\r\nhttps://github.com/jsdom/jsdom/issues/2555\r\n\r\n---------\r\n\r\nCo-authored-by:
Thomas Watson <w@tson.dk>\r\nCo-authored-by: kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"e9a651933768353e65c2c4e52b98b768bb7f3b27"}},{"url":"#162351"}]}]
BACKPORT-->
dgieselaar pushed a commit to dgieselaar/kibana that referenced this issue Jul 23, 2023
This PR adds the `Blob` polyfill into the jest jsdom env which is
currently not supported as mentioned in
jsdom/jsdom#2555

---------

Co-authored-by: Thomas Watson <w@tson.dk>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this issue Aug 1, 2023
This PR adds the `Blob` polyfill into the jest jsdom env which is
currently not supported as mentioned in
jsdom/jsdom#2555

---------

Co-authored-by: Thomas Watson <w@tson.dk>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@rmuratov
Copy link

rmuratov commented Sep 6, 2023

For vitest, import 'blob-polyfill' in setup.ts helped.

@solovevserg
Copy link

I agree with jimmywarting, that using native Node.js Blob seems the right option. Currently it's possible to override JSDom implementation with native Node.js implementation in setup-jest.ts like so

import {Blob as BlobPolyfill} from 'node:buffer';

global.Blob = BlobPolyfill as any;

I checked that it works in Node 18.18.2

undergroundwires added a commit to undergroundwires/privacy.sexy that referenced this issue Jan 13, 2024
This commit introduces native operating system file dialogs in the
desktop application replacing the existing web-based dialogs.

It lays the foundation for future enhancements such as:

- Providing error messages when saving or executing files, addressing
  #264.
- Creating system restore points, addressing #50.

Documentation updates:

- Update `desktop-vs-web-features.md` with added functionality.
- Update `README.md` with security feature highlights.
- Update home page documentation to emphasize security features.

Other supporting changes include:

- Integrate IPC communication channels for secure Electron dialog API
  interactions.
- Refactor `IpcRegistration` for more type-safety and simplicity.
- Introduce a Vue hook to encapsulate dialog functionality.
- Improve errors during IPC registration for easier troubleshooting.
- Move `ClientLoggerFactory` for consistency in hooks organization and
  remove `LoggerFactory` interface for simplicity.
- Add tests for the save file dialog in the browser context.
- Add `Blob` polyfill in tests to compensate for the missing
  `blob.text()` function in `jsdom` (see jsdom/jsdom#2555).

Improve environment detection logic:

- Treat test environment as browser environments to correctly activate
  features based on the environment. This resolves issues where the
  environment is misidentified as desktop, but Electron preloader APIs
  are missing.
- Rename `isDesktop` environment identification variable to
  `isRunningAsDesktopApplication` for better clarity and to avoid
  confusion with desktop environments in web/browser/test environments.
- Simplify `BrowserRuntimeEnvironment` to consistently detect
  non-desktop application environments.
- Improve environment detection for Electron main process
  (electron/electron#2288).
@silverwind
Copy link

What's interesting is that one can obtain Blobs that have the missing methods from the fetch api:

const blob = await (await globalThis.fetch('data:text/plain;utf8,test')).blob();
console.log(await blob.stream());
console.log(await blob.text());
console.log(await blob.arrayBuffer());

output:

ReadableStream { locked: false, state: 'readable', supportsBYOB: true }
test
ArrayBuffer { [Uint8Contents]: <74 65 73 74>, byteLength: 4 }

This is in vitest using jsdom environment. I assume the fetch in use here is from Node. I did not polyfill anything here.

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

No branches or pull requests