Update image-asset-common.ts#10894
Update image-asset-common.ts#10894Ayush-Raj-Chourasia wants to merge 2 commits intoNativeScript:mainfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit b9ee846
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull Request Overview
This PR adds string-to-number coercion logic for width and height parameters in the getRequestedImageSize function. While the ImageAssetOptions interface specifies these as optional numbers, this change adds runtime handling to convert string values to numbers if they are provided.
- Added type checking and parsing for width and height values that may be strings
- Maintained existing aspect ratio calculation and dimension logic
- Applied consistent indentation changes to the function body
Comments suppressed due to low confidence (1)
packages/core/image-asset/image-asset-common.ts:1
- The height parsing is using
reqWidthinstead ofreqHeighton line 59. This should beparseInt(reqHeight, 10).
import { ImageAsset as ImageAssetDefinition, ImageAssetOptions } from '.';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reqWidth = parseInt(reqWidth, 10); | ||
| } | ||
| if (typeof reqHeight === 'string') { | ||
| reqHeight = parseInt(reqHeight, 10); |
There was a problem hiding this comment.
The ImageAssetOptions interface defines width and height as optional number types, but this code suggests they could be strings at runtime. Consider using Number() or adding explicit NaN validation after parseInt(), as parseInt() can return NaN for invalid strings, which would propagate through calculations.
| reqWidth = parseInt(reqWidth, 10); | |
| } | |
| if (typeof reqHeight === 'string') { | |
| reqHeight = parseInt(reqHeight, 10); | |
| const parsedWidth = parseInt(reqWidth, 10); | |
| reqWidth = isNaN(parsedWidth) ? Math.min(src.width, Screen.mainScreen.widthPixels) : parsedWidth; | |
| } | |
| if (typeof reqHeight === 'string') { | |
| const parsedHeight = parseInt(reqHeight, 10); | |
| reqHeight = isNaN(parsedHeight) ? Math.min(src.height, Screen.mainScreen.heightPixels) : parsedHeight; |
| let reqWidth = options.width || Math.min(src.width, Screen.mainScreen.widthPixels); | ||
| let reqHeight = options.height || Math.min(src.height, Screen.mainScreen.heightPixels); | ||
|
|
||
| return { | ||
| width: reqWidth, | ||
| height: reqHeight, | ||
| }; | ||
| if (typeof reqWidth === 'string') { | ||
| reqWidth = parseInt(reqWidth, 10); | ||
| } | ||
| if (typeof reqHeight === 'string') { | ||
| reqHeight = parseInt(reqHeight, 10); | ||
| } |
There was a problem hiding this comment.
The string coercion checks on lines 55-60 occur after using the fallback logic on lines 52-53. If options.width or options.height are strings, the || operator won't trigger the fallback because non-empty strings are truthy. The string type checks should occur before or be integrated with the fallback logic to ensure proper behavior.
|
Hey! Thanks so much for opening a pull request! A prior PR was opened to solve this issue here: #10862 which will be merging. Thanks again, and we look forward to seeing more PRs from you in the future! ❤️ |
2 participants
fix(android): coerce string width/height in ImageAssetOptionswidthorheightof ImageAssetOptions results in an error on android #6289What is the current behavior?
Setting a string value (e.g.,
"300") forwidthorheightinImageAssetOptionscauses a crash on Android API level 22 with the error:java.lang.IllegalArgumentException: bitmap size exceeds 32 bits.This happens because the string is not coerced to a number, resulting in an invalid bitmap size.
What is the new behavior?
The function
getRequestedImageSizenow coerceswidthandheightto numbers if they are passed as strings. This prevents the bitmap size error on Android and ensures compatibility with older API levels.Fixes/Implements/Closes #6289.