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

[ASSETS-36125] Update SVG creation logic #216

Merged
merged 13 commits into from
Mar 18, 2024
2 changes: 1 addition & 1 deletion .github/workflows/node.js.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
if: "!contains(github.event.head_commit.message, '[ci skip]')"
strategy:
matrix:
node-version: [14.17]
node-version: [18.4.0]
Copy link
Member Author

@tmathern tmathern Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even more tests hang/fail without this update, because when installing some libs warn of a bad (outdated) node engine.


steps:
- uses: actions/checkout@v2
Expand Down
14 changes: 11 additions & 3 deletions lib/postprocessing/image.js
Original file line number Diff line number Diff line change
Expand Up @@ -634,13 +634,21 @@ async function imagePostProcess(intermediateRendition, rendition, directories) {

function convertSvg(img, instructions) {
// ImageMagick automatically keeps aspect ratio
// Only using width because ImageMagick will use the smallest value whether it be width or height
const width = instructions.width || SVG_DEFAULT_WIDTH;
let imageSize = SVG_DEFAULT_WIDTH;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be used only if nothing is set in instructions at all.


if(instructions.width && instructions.height) {
// image magick will keep aspect ratio
imageSize = `${instructions.width}x${instructions.height}`;
} else if(instructions.width) {
imageSize = `${instructions.width}x`;
Copy link
Member Author

@tmathern tmathern Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The x is left for clarity in the command param, to know we got width only (width is considered to be by default the first value in the command param).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to log wether we only got with or height or got both instead of having to examine the cmd?

} else if(instructions.height) {
imageSize = `x${instructions.height}`;
Copy link
Member Author

@tmathern tmathern Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The x is left for clarity in the command param to know we got height only, and to make sure aspect ratio is kept, since height is the second param (and the first param, width, is "empty").

}

img = handleTransparency(img, instructions);

// some svgs have no size (only percentage width/height), so we scale them to our target size
img.in("-size", `${width}`); // 2020-11-10 img.rawSize() will not be applied at the correct time and the SVG will be upscaled resulting in a fuzzy final rendition
img.in("-size", `${imageSize}`); // 2020-11-10 img.rawSize() will not be applied at the correct time and the SVG will be upscaled resulting in a fuzzy final rendition
return img;
}

Expand Down
Copy link
Member Author

@tmathern tmathern Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems there was no (visible) change in this rendition

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one change - logic change leads to width being 1 px smaller.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one change - logic change leads to width being 1 px smaller.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 2 additions & 1 deletion test/api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,8 @@ describe("api.js", () => {
assert.ok(!fs.existsSync(renditionDir));
});

it('should send metrics - rendition and activation with cgroup metrics', async () => {
it.skip('should send metrics - rendition and activation with cgroup metrics', async () => {
Copy link
Member Author

@tmathern tmathern Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flaky: sometimes (most often) hangs, sometimes runs. Keeping for local testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this hanging happening in prod too?

// can hang in CI/CD
const receivedMetrics = MetricsTestHelper.mockNewRelic();
mockFs({
'/sys/fs/cgroup': {
Expand Down
Loading