Fix/dpp 125 #159

sksadjad · 2024-02-09T06:53:08Z

fixed the problem with image-size. the version of image-size previously used was not react friendly

codecov-commenter · 2024-02-09T06:57:07Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (ba38097) 45.89% compared to head (a59fdcf) 33.92%.
Report is 763 commits behind head on develop.

❗ Current head a59fdcf differs from pull request most recent head 11f870c. Consider uploading reports for the commit 11f870c to get more accurate results

Files	Patch %	Lines
packages/ssi-sdk-core/src/utils/image.ts	84.84%	5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop     #159       +/-   ##
============================================
- Coverage    45.89%   33.92%   -11.98%     
============================================
  Files           65      151       +86     
  Lines         2412     7216     +4804     
  Branches       539     1698     +1159     
============================================
+ Hits          1107     2448     +1341     
- Misses        1305     4767     +3462     
- Partials         0        1        +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nklomp

See remarks. Espacially the toString is too of a naive implementation when working with images

nklomp · 2024-02-10T00:37:40Z

packages/ssi-sdk-core/src/utils/image.ts

+  type?: string
+}
+
+const uint8ArrayToString = (uint8Array: Uint8Array): string => {


Please use the uint8arrays package for that as we are using that in everywhere. It has a toString method with encoding support

changed to use that library

nklomp · 2024-02-10T00:39:44Z

packages/ssi-sdk-core/src/utils/image.ts

+
+// TODO: here we're handling svg separately, remove this section when image-size starts supporting it in version 2
+const isSvg = (uint8Array: Uint8Array): boolean => {
+  const text = uint8ArrayToString(uint8Array)


Can't we detect this based in the array itself, or at least only take a few indices first. This probably has a massive hit on performance. This for instance will covert 10 MB images into text first, only to determine first whether it is a SVG or not from only the first few chars

yeah, changed the code to use only first bytes

nklomp · 2024-02-10T00:40:28Z

packages/ssi-sdk-core/src/utils/image.ts

+
+const getSvgDimensions = (uint8Array: Uint8Array) => {
+  const svgContent = uint8ArrayToString(uint8Array)
+  const widthMatch = svgContent.match(/width="([^"]+)"/)


Does this work at all? For a simple SVG probably it might, but what about an SVG with all kinds of objects in it?

changed this method and wrote 2 additional tests for a multi-layered and a animated svg

nklomp · 2024-02-10T00:43:27Z

packages/ssi-sdk-core/src/utils/image.ts

+  const heightMatch = svgContent.match(/height="([^"]+)"/)
+  const viewBoxMatch = svgContent.match(/viewBox="([^"]+)"/)
+
+  let width, height


Why are these not typed?

changed these and made everything typed

nklomp · 2024-02-10T00:44:13Z

packages/ssi-sdk-core/src/utils/image.ts

+  if ((!width || !height) && viewBoxMatch) {
+    const parts = viewBoxMatch[1].split(' ').map(Number)
+    if (!width && parts.length === 4) {
+      width = parts[2].toString()


Why a tostring on something that is mapped to a number?

changed this, width/height is a number now

nklomp · 2024-02-10T00:45:40Z

packages/ssi-sdk-core/src/utils/image.ts

 }

 export const getImageDimensions = async (base64: string): Promise<IImageDimensions> => {
  const buffer: Buffer = Buffer.from(base64, 'base64')
-  const dimensions: ISizeCalculationResult = sizeOf(buffer)
+  const dimensions: SizeCalculationResult = isSvg(buffer) ? getSvgDimensions(buffer) : imageSize(buffer)


Just create a getImageDimensions function, so other places can also reuse it

we already have getImageDimensions if the image is svg, we're calling a special method called getSvgDimensions which is not exported. Do you want this to be also exported?

Okay then it does make sense to also add an uint8array next to the string for this function

now it's accepting both base64 string and uint8array. also added explanation at the top of the method

also applied this to the getImageMediaType function. now it's also accepting uint8array type

nklomp · 2024-02-14T23:58:53Z

packages/ssi-sdk-core/src/utils/image.ts

  }
+  const result: SizeCalculationResult = imageSize(buffer)
+  return `image/${result.type}`
 }

 export const getImageDimensions = async (base64: string): Promise<IImageDimensions> => {
  const buffer: Buffer = Buffer.from(base64, 'base64')


We avoid Buffer by default because of RN. Use U8Int Array which is compatible, simply use the u8a.fromString method

changed this and one other place (in this file) that we're using buffer and used u8a.fromString(value, 'base64') instead.

… Uint8Array. plus some code style changes in the core package

# Conflicts: # packages/ssi-sdk-core/package.json # pnpm-lock.yaml

sksadjad added 3 commits February 8, 2024 22:57

fix: changed image-size library version to a react friendly one

308bad7

fix: changed the image-size usage to handle uint8array

1a0e080

fix: fixed the svg problem with image-size

d7823eb

nklomp requested changes Feb 10, 2024

View reviewed changes

sksadjad added 2 commits February 14, 2024 12:24

fix: modified handling svg files in ssi-sdk.core

c86188e

chore: updated pnpm-lock.yaml

9bc0685

nklomp reviewed Feb 14, 2024

View reviewed changes

sksadjad added 5 commits February 16, 2024 10:12

refactor: changed usages of buffer to uint8arrays in the core's image

608c29f

doc: added doc for getImageDimensions explaining the input type in core

c420a2d

refactor: changed the input type for getImageMediaType to also accept…

11f870c

… Uint8Array. plus some code style changes in the core package

Merge branch 'develop' into fix/DPP-125

67642a1

# Conflicts: # packages/ssi-sdk-core/package.json # pnpm-lock.yaml

chore: pnpm lock update

5cb8b6b

nklomp approved these changes Feb 22, 2024

View reviewed changes

sksadjad merged commit 225b2a3 into develop Feb 22, 2024
2 checks passed

sksadjad deleted the fix/DPP-125 branch February 22, 2024 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/dpp 125 #159

Fix/dpp 125 #159

sksadjad commented Feb 9, 2024

codecov-commenter commented Feb 9, 2024 •

edited

Loading

nklomp left a comment

nklomp Feb 10, 2024

sksadjad Feb 14, 2024

nklomp Feb 10, 2024 •

edited

Loading

sksadjad Feb 14, 2024

nklomp Feb 10, 2024

sksadjad Feb 14, 2024

nklomp Feb 10, 2024

sksadjad Feb 14, 2024

nklomp Feb 10, 2024

sksadjad Feb 14, 2024

nklomp Feb 10, 2024

sksadjad Feb 14, 2024

nklomp Feb 14, 2024

sksadjad Feb 16, 2024

sksadjad Feb 16, 2024

nklomp Feb 14, 2024 •

edited

Loading

sksadjad Feb 16, 2024

Fix/dpp 125 #159

Fix/dpp 125 #159

Conversation

sksadjad commented Feb 9, 2024

codecov-commenter commented Feb 9, 2024 • edited Loading

Codecov Report

nklomp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nklomp Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nklomp Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 9, 2024 •

edited

Loading

nklomp Feb 10, 2024 •

edited

Loading

nklomp Feb 14, 2024 •

edited

Loading