TonyBrobston / jpegasus

A lean client-side JavaScript image compressor.
MIT License
11 stars 2 forks source link

Compression changes object given #173

Closed WesleyHovick closed 4 years ago

WesleyHovick commented 4 years ago

Legacy Edge & IE11 return blobs instead of files when the input is a file. When given a File object as an input, compress returns a Blob in Legacy Edge and IE11. This is a problem because the expected input file has a name and lastModifiedDate property, but after compression does not. This behavior is not observed in Google Chrome or the recently released Chromium based Edge.

TonyBrobston commented 4 years ago

Hey @WesleyHovick , thanks for reporting this issue.

Do you have a suggested solution?

This File/Blob situation was intentional. I marked the return type as File|Blob https://github.com/TonyBrobston/jpegasus/blob/214402ef221aa52a13bccc2a46cca4734314f013/src/index.ts#L8

I was between a rock and hard spot when making this decision a year or so ago. File seems to be the type to use, however File does not exist in IE11. File extends Blob, so Blob is almost a viable replacement; but as you are mentioning, it is not the same thing. It seems like the alternative option is to always return Blob. Maybe there is another option?

I'm curious to hear your thoughts.

TonyBrobston commented 4 years ago

Now that I think back, I'm not sure which version of IE doesn't have File. It may be all of them, but it may not; I don't recall.

TonyBrobston commented 4 years ago

I believe this is what we're looking for. https://developer.mozilla.org/en-US/docs/Web/API/File

WesleyHovick commented 4 years ago

@TonyBrobston Sorry for the late reply, super busy!

You are completely correct File extends Blob. I also noticed that you specified the return type of File | Blob, and I think in almost all cases this is perfectly okay.

However, in our case we are actually using the compressed file with our frontend to show the name of the file, which is how we came across this issue with our IE11 and Edge users. We do some parsing of the name of the file which is what blew up when we tried to use .name and parse the string.

I'll try and take a closer look at the code when I get the chance to see if I have any recommendations.

My first thoughts are as follows: If I'm not mistaken in my original post, I think the only difference between a Blob and a File (besides the obvious type difference) is that Files have a name and a lastModifiedDate. I'm wondering if compress could preserve these values and then just add them to the Blob object before returning it. Even though the object type is technically different, that would take care of any IE or Edge situations such as mine.

Let me know your thoughts!

TonyBrobston commented 4 years ago

@WesleyHovick No rush, I'm just trying to be timely in hopes that you will continue to use this package :).

I think we're on the same page. I don't think there's anything we can do about the type, but I am pretty sure we can preserve name. I'll look into what other properties File has and see if it's possible to preserve them.

I think this is will be a good change and probably should have been there since the beginning.

I'll likely have something together today.

TonyBrobston commented 4 years ago

So I did just realize one potential issue.

File:

> const file = new File([1234], 'foo');
undefined
> file
File {name: "foo", lastModified: 1601648999225, lastModifiedDate: Fri Oct 02 2020 09:29:59 GMT-0500 (Central Daylight Time), webkitRelativePath: "", size: 4, …}
> file.name
"foo"

Blob:

> const blob = new Blob([1234]);
undefined
> blob
Blob {size: 4, type: ""}
> blob.name = 'foo'
"foo"
> blob
Blob {name: "foo", size: 4, type: ""}
> blob.name
"foo"

I can definitely implement something like above here: https://github.com/TonyBrobston/jpegasus/blob/214402ef221aa52a13bccc2a46cca4734314f013/src/services/fileService.ts#L9-L15

const create = (bytes: Uint8Array[], type: string, name: string): File|Blob => {
    try {
        return new File(bytes, name, {type});
    } catch (error) {
        const blob = new Blob(bytes, {type});
        blob.name = name;
        return blob;
    }
};

However, the original file being passed in likely does not have a name in the case of Blob. So I'm thinking blob.name would get set to undefined or null or something along those lines.

I also just realized that the input type is currently File and should probably be File|Blob since File does not exist in some browsers. https://github.com/TonyBrobston/jpegasus/blob/214402ef221aa52a13bccc2a46cca4734314f013/src/index.ts#L8

I guess the thing that needs to be figured out is; is it possible to get a Blob from an input field like this one:

<input type="file" id="imageInput" />

(https://github.com/TonyBrobston/jpegasus-demo/blob/b636a4a4cf9ff7ad14f64fcf0ff9747e2a57500e/src/index.html#L564) and still get the name from that blob?

TonyBrobston commented 4 years ago

Alright, I did some testing in ie11 and the path forward is clear. ie11 does not have the File constructor. But when a file is loaded into a input of type file like this:

<input type="file" id="imageInput" />

Then it really is a File and has a name.

TonyBrobston commented 4 years ago

Here is what I have so far. https://github.com/TonyBrobston/jpegasus/pull/174

Since name seems to be the most important, I'll probably merge/deploy that and then look into the other properties.

TonyBrobston commented 4 years ago

Publishing a release, should be deployed shortly. https://github.com/TonyBrobston/jpegasus/releases/tag/1.15.1

TonyBrobston commented 4 years ago

I added a console.info to my demo site (https://tonybrobston.github.io/jpegasus-demo) and am able to see that Blob now has name.

TonyBrobston commented 4 years ago

Also adding lastModified and lastModifiedDate. lastModifiedDate is deprecated, but I added it anyhow.

https://github.com/TonyBrobston/jpegasus/pull/177

TonyBrobston commented 4 years ago

Alright, ^ is released. https://github.com/TonyBrobston/jpegasus/releases/tag/1.15.2

WesleyHovick commented 4 years ago

@TonyBrobston Awesome! Great work, I'll pull in the new version when I get the chance and remove my workaround and see if just using this works now in IE11 and Legacy Edge. Thanks! 😸

TonyBrobston commented 4 years ago

@WesleyHovick No problem, glad to help.

Let me know if you have any problems. Also, if you have any other feedback on the experience of using this package I'd appreciate any feedback you can give.

WesleyHovick commented 4 years ago

@TonyBrobston Pulled in latest, reverted my work around, and it works great in IE11. Thanks again for your quick responses and quick fix! 😸