fix: webp detection #63
Conversation
🦋 Changeset detectedLatest commit: 753fc9d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
There was a problem hiding this comment.
Overall, it looks good, but it raises a question I have in mind: should we fail or throw an error instead of assuming the file type is PNG if we can't detect its mime_code against the map of all known mime_codes?
Also It would also be good to conduct a quick verification of the existing mime_code signatures to include the full signature of the file we're verifying, rather than just the first few bytes of the entire signature.
What do you think?
| @@ -38,10 +38,10 @@ export const MIME_CODES: MimeCodes = { | |||
| png: [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a], | |||
| gif: [0x47, 0x49, 0x46], | |||
| bmp: [0x42, 0x4d], | |||
| webp: [0x52, 0x49, 0x46, 0x46, 0x57, 0x45, 0x42, 0x50], | |||
| webp: [0x52, 0x49, 0x46, 0x46, 0x00, 0x00, 0x00, 0x00, 0x57, 0x45, 0x42, 0x50], | |||
There was a problem hiding this comment.
[suggestion]
Replace 0x00 with another placeholder to explicitly indicate that these values are not necessary for the MIME type check. For example:
webp: [0x52, 0x49, 0x46, 0x46, "?", "?", "?", "?", 0x57, 0x45, 0x42, 0x50],| const riff = Array.from(bufData.subarray(0, 4)); | ||
| const webpCheck = Array.from(bufData.subarray(8, 12)); |
There was a problem hiding this comment.
| const riff = Array.from(bufData.subarray(0, 4)); | |
| const webpCheck = Array.from(bufData.subarray(8, 12)); | |
| const riffHeader = Array.from(bufData.subarray(0, 4)); | |
| const webpFormatMarker = Array.from(bufData.subarray(8, 12)); |
There was a problem hiding this comment.
done @theashraf
| @@ -358,17 +358,17 @@ export class DotLottie extends DotLottieCommon { | |||
| } | |||
| } | |||
| } | |||
| } catch (err) { | |||
| } catch (err: any) { | |||
There was a problem hiding this comment.
isn't suppose to be instance of Error ?
There was a problem hiding this comment.
done @theashraf
| 'image_4.png', | ||
| 'image_5.png', | ||
| 'image_9.png', | ||
| ]); | ||
| expect(uniqueImages.map((image) => image.id)).toEqual(['image_1', 'image_2', 'image_4', 'image_5', 'image_9']); | ||
| }); | ||
| }); | ||
|
|
||
| it('Properly detects mimetype of images.', async () => { |
There was a problem hiding this comment.
[suggestion]
Add unit tests for getMimeTypeFromBase64 to cover the logic for detecting all supported MIME types. The responsibility for preventing similar issues in the future lies with getMimeTypeFromBase64.
Since the logic for inlining assets is already covered by other tests, I don't see the value in adding more integration tests to verify if getMimeTypeFromBase64 is functioning as expected.
There was a problem hiding this comment.
added test for all supported file type @theashraf
| try { | ||
| let videoDotLottie = new DotLottie(); | ||
|
|
||
| videoDotLottie = await videoDotLottie.fromArrayBuffer(VIDEO_DOTLOTTIE); | ||
|
|
||
| const videoAnimation = videoDotLottie.getAnimations(); | ||
|
|
||
| if (videoAnimation) { | ||
| videoAnimation.map(async (animation) => { | ||
| await animation[1].toJSON({ | ||
| inlineAssets: true, | ||
| }); | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(Error); | ||
| } | ||
| }); |
There was a problem hiding this comment.
[suggestion]
Consider moving this assertion to an independent test case, as it checks for an error when an unsupported MIME type is used. This differs from the primary intention of the test case, which is to assert the happy path.
There was a problem hiding this comment.
done @theashraf
No description provided.