Skip to content

Conversation

@moonglum
Copy link
Member

@moonglum moonglum commented Mar 9, 2025

  • This uses the new utils that faucet-pipeline-assets exposes. Works really well, and removes a lot of code from this package that has nothing to do with image compression. With this, it is compatible with both core 2 and 3.
  • In the first attempt, I tried to add JSDoc comments type checked by TypeScript. But this failed, as JSDoc comments can't import types from another package. I kept the JSDoc comments that still made sense to me.

Copy link
Contributor

@FND FND left a comment

Choose a reason for hiding this comment

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

This looks promising indeed! 👍

@import not supporting third-party types is indeed baffling and somewhat concerning. As a workaround, we could import and re-export those types in types.ts though?

Some of the suggested changes from faucet-pipeline/faucet-pipeline-static#71 apply here as well (e.g. Y U NO ESM, explicit paths with ./, TypeScript configuration, @import formatting), but I opted not to add redundant noise here while we're still discussing such nuances over there.

lib/index.js Outdated
return output.data;
} catch(error) {
abort(`Only SVG can be converted to SVG: ${sourcePath}`);
return ""; // XXX: this is for typescript :joy:
Copy link
Contributor

@FND FND Mar 16, 2025

Choose a reason for hiding this comment

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

@ts-expect-error perhaps? But yeah, annoying...

Also, you might want to use repr in such error messages. (Same elsewhere, possibly also in faucet-static? I don't recall at this moment.)

image.resize({
width: metadata.width * scale,
height: metadata.height * scale
});
Copy link
Contributor

Choose a reason for hiding this comment

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

YMMV, but destructuring could make this a little less verbose and thus more readable?

let { width, height } = await image.metadata();
if(width && height) {
	image.resize({
		width: width * scale,
		height: height * scale
	});
}

In fact, this made me realize a potential bug: Should we worry about zero values in that if statement? You never know what kind of weird input people have...

lib/index.js Outdated

// extname follows this annoying idea that the dot belongs to the extension
/**
* extname follows this annoying idea that the dot belongs to the extension
Copy link
Contributor

Choose a reason for hiding this comment

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

As a ranty comment, this seemed fine - as a docstring, it seems more confusing than helpful? At least I had to spend a few cycles trying to decode the intent here.

However, I believe faucet-static does the same thing, so perhaps that should become a reusable utility (i.e. it shouldn't be inlined after all 🙈 )?

@moonglum
Copy link
Member Author

@import not supporting third-party types is indeed baffling and somewhat concerning. As a workaround, we could import and re-export those types in types.ts though?

You misunderstood that. The types I'm importing are in faucet-pipeline-static/lib/types.ts. But you can't import them from a comment... 😭 For me, this puts a giant question mark into the "TS Doc" effort in these two projects, and I'm considering removing the TS stuff in both...

@moonglum moonglum changed the title Using the new static utils Release 2.3.0 Apr 22, 2025
@moonglum moonglum force-pushed the static branch 3 times, most recently from d104612 to 31dbf28 Compare April 23, 2025 06:48
@moonglum moonglum requested a review from FND April 23, 2025 06:49
@FND
Copy link
Contributor

FND commented May 1, 2025

I've created a minimal test case for the JSDoc imports issue:

src/index.js
/** @type {Sample} */
let data = {
	abc: 123,
	def: 456,
	ghi: "…"
};
console.log(data);

/** @import { Sample } from "sample/src/index.types.ts" */
node_modules/sample/src/index.types.ts
export type Sample = Record<string, number>;
tsconfig.json
{
    "compilerOptions": {
        "allowJs": true,
        "checkJs": true,
        "noEmit": true,
        "strict": true,
        "erasableSyntaxOnly": true,
        "isolatedModules": true,
        "allowImportingTsExtensions": true
    }
}
$ npm install typescript
$ ./node_modules/.bin/tsc -p ./tsconfig.json
src/index.js:5:2 - error TS2322: Type 'string' is not assignable to type 'number'.

5  ghi: "…"
   ~~~

Found 1 error in src/index.js:5

That seems to work as expected - what am I missing?

@moonglum moonglum marked this pull request as ready for review November 2, 2025 13:30
@moonglum
Copy link
Member Author

moonglum commented Nov 2, 2025

What I did here:

  • I reset the main branch to the last release (2.2)
  • I then took all the commits since 2.2 plus the commits on this branch, and then cleaned up the commits

I think this is ready to become a 2.3 release, dropping the need for FileFinder in core, reducing and updating dependencies.

Caveat: This does not update svgo, I need to sit down and understand the differences before I do that #83

@FND
Copy link
Contributor

FND commented Nov 3, 2025

Aside: My JSDoc imports MTC still works on my end. 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants