v2.2.0 #71

moonglum · 2025-03-08T21:00:04Z

Goals of this PR:

Make this plugin compatible with core v2 and v3 by vendoring FileFinder
Make it possible for -images to build on top of this

Breaking Change:

If we want to support ESM configurations, we need to rename this plugin as 'static' is a reserved word.
Revert the change to require 22, and instead require 20.19.0 as a minimum version once it is released

While doing it, two secondary goals emerged:

Simplify FileFinder as suggested in simplified FileFinder utility faucet-pipeline-core#148
Add types to make the refactoring safer
ESM

FND

This is good work - thank you!

Here's a first-pass review; mostly just FNDian details.

index.js

types.ts

index.js

lib.js

index.js

moonglum · 2025-03-09T16:59:20Z

While transforming this module, I ran into a major problem 🙈 I think we need to rename this plugin 😅

If you do what we say on our homepage, it will not work:

export const static = [{                                                                                                                                              
    source: "./src",                                                                                                                                                    
    target: "./dist"                                                                                                                                                    
}];

This leads to this error:

export const static = [{
             ^^^^^^

SyntaxError: Unexpected strict mode reserved word

Turns out: Reserved words are not the best identifiers. I guess we need to deprecate faucet-pipeline-static and rename it to faucet-pipeline-files (or faucet-pipeline-copy): I implemented this in b8cbb64.

Sidenote:
As you can see, it fails on node 20 but works in 22 🙈 This is because the backport of require(ESM) will be released next week:
nodejs/node#57349

moonglum · 2025-03-09T18:42:19Z

Okay, so I'm done with the contract now. This allows images to build on top of this pipeline. The exposed utility is a single function: buildProcessPipeline. It handles all the shenanigans of handling a directory of files as an input (leveraging FileFinder). It takes a function processFile that is specific to the pipeline and processes a single file. That function takes the filename, source, target, targetFolder and the assetManager. The function will also write the file. Why? Because in the case of images, for example, a single input file could produce multiple output files.

Looking forward to your feedback, @FND 🙇

moonglum · 2025-03-09T20:35:10Z

Created the first version of images that uses the utils exported here:
faucet-pipeline/faucet-pipeline-images#77

* Improvements to FileFinder * Doc improvement * fileName => filename * determineCompactors * Inline processFiles * Code organization * Remove last `then` * Inline isDotfile

moonglum · 2025-03-15T17:44:44Z

lib/index.js

+import { readFile } from "node:fs/promises";
+import * as path from "node:path";
+
+export const key = "static";


@FND Do you remember what this name is actually used for? I'm not sure it is used for plugins that are in the default list, only for the ones that are not:
https://github.com/faucet-pipeline/faucet-pipeline-core/blob/main/lib/plugins.js#L5

I was confused by this as well and haven't investigated it yet (or rather: I don't understand our implementation anymore without investing a fair amount of effort), but tests suggest that a plugin can override faucet-core's defaults. I'm not 100 % sure that interpretation is correct tough.

But yes, this value is most likely inert unless it differs from faucet-core's baked-in defaults. It still seems proper for plugins to be self-descriptive though.

I think you don't really need this option for the plugins that are defined in core, but only for those that are not.

FND

This is excellent work, thank you!

I only have a few more observations, most of it details or just generally sharing perspectives and expressing my own uncertainties. None of it seems worth blocking, so proceed on your own recognizance.

(I have to admit it's quite possible that the whole type stuff distracted me from actually reviewing the core functionality: While I've come to accept types as useful in some circumstances, as in this case, they also tend to make reviews much more onerous.)

FND · 2025-03-16T08:53:54Z

lib/index.js

+import { readFile } from "node:fs/promises";
+import * as path from "node:path";
+
+export const key = "static";


I was confused by this as well and haven't investigated it yet (or rather: I don't understand our implementation anymore without investing a fair amount of effort), but tests suggest that a plugin can override faucet-core's defaults. I'm not 100 % sure that interpretation is correct tough.

But yes, this value is most likely inert unless it differs from faucet-core's baked-in defaults. It still seems proper for plugins to be self-descriptive though.

lib/index.js

lib/types.ts

FND · 2025-03-16T09:18:48Z

lib/types.ts

+
+export interface Filter {
+	(filename: string): boolean
+}


FWIW, I generally try to keep things local to the respective file, i.e. *.ts is a last resort for me.

In this case, I would have added the following to the bottom of util.js:

/** @typedef {{ skipDotfiles: boolean, filter?: Filter }} FileFinderOptions */

This not only reduces indirections (which can become quite taxing for the mind), it's also in line with deletability and means you need less expansive @import statements. In fact, I would argue that the need for multi-line @import is a potential smell.

That smell also suggests we might want to move FileFinder to a separate module (finder.js?) and break up types.ts into separate modules (e.g. util.types.ts and finder.types.ts) - that, too, comes down to deletability.

However, that's just me; YMMV.

package.json

FND · 2025-03-16T09:37:57Z

tsconfig.json

+		"strict": true,
+		"allowImportingTsExtensions": true,
+		"lib": ["dom", "es2023"],
+		"module": "nodenext"


🤔 Are allowImportingTsExtensions and module necessary?

That's a genuine question: My JSDoc-typed projects exclusively use Deno for type checking, so I'm never entirely sure whether regular TypeScript works exactly the same way.

Relatedly, I've long been wondering whether TypeScript configuration should include isolatedModules and now erasableSyntaxOnly (though the latter should only be relevant for .ts source code anyway).

Basically, I'm perennially insecure about the non-obvious nuances of TypeScript configuration (module is a good example of that), so I obsessively try to reduce it to the bare minimum.

allowImportingTsExtensions: If I remove it, TS fails 😅 From my understanding this is because we are importing ts files from the comments of JS files.

module option: Not adding it leads to the error Module '"node:path"' can only be default-imported using the 'esModuleInterop' flag

Activated the two options 👍

Roger, thanks. (That suggests using Deno for type checking is not a great idea because obscure abstractions vs. using the respective tool directly.)

tsconfig.json

moonglum · 2025-04-23T09:21:26Z

I think the remaining question for static is, if it is worth it to keep the types around. As explained in the images PR, it doesn't really work for our setup :(

moonglum · 2025-10-27T07:40:26Z

I want to close this in favor of faucet-pipeline-assets, @FND. I do not plan to update faucet-pipeline-static for core 3.x.

moonglum added 2 commits March 8, 2025 19:21

Bump required node version, remove npm-run-all

23eeb22

Vendor FileFinder to make this dual compatible, type the code

e3f330e

FND reviewed Mar 9, 2025

View reviewed changes

index.js Outdated Show resolved Hide resolved

moonglum force-pushed the dual-versions branch from 50c87b7 to 400b0bb Compare March 9, 2025 17:06

FND mentioned this pull request Mar 13, 2025

plugin name constitutes a reserved identifier, blocking ESM compatibility #72

Open

moonglum mentioned this pull request Mar 15, 2025

faucet-pipeline-core 3.1.0 rollout faucet-pipeline/faucet-pipeline-core#149

Open

6 tasks

Apply FND's suggestions

b5f35be

* Improvements to FileFinder * Doc improvement * fileName => filename * determineCompactors * Inline processFiles * Code organization * Remove last `then` * Inline isDotfile

moonglum force-pushed the dual-versions branch from 37955bd to 10f9182 Compare March 15, 2025 16:58

moonglum added 3 commits March 15, 2025 18:01

Set the minimum Node version to 20.19.0

ac3bda6

ESM

c9522ba

Make this pipeline the basis for other file processors

4e0bbb3

moonglum force-pushed the dual-versions branch from 22f7420 to 4e0bbb3 Compare March 15, 2025 17:26

moonglum requested a review from FND March 15, 2025 17:29

moonglum mentioned this pull request Mar 15, 2025

simplified FileFinder utility faucet-pipeline/faucet-pipeline-core#148

Closed

moonglum commented Mar 15, 2025

View reviewed changes

FND approved these changes Mar 16, 2025

View reviewed changes

This was referenced Mar 16, 2025

Release 2.3.0 faucet-pipeline/faucet-pipeline-images#77

Draft

v4.0.0 faucet-pipeline/faucet-pipeline-core#150

Draft

moonglum added 3 commits April 22, 2025 13:34

FND appeasements

5f5e8f0

Improve tsconfig

25e8ef9

v2.2.0

06b436b

moonglum changed the title ~~Dual core~~ v2.2.0 Apr 23, 2025

moonglum marked this pull request as ready for review April 23, 2025 09:20

moonglum requested a review from FND April 23, 2025 09:21

Use the types.ts from core

cd249cb

moonglum mentioned this pull request Oct 23, 2025

2.1 faucet-pipeline/faucet-pipeline-core#163

Merged

v2.2.0 #71

Are you sure you want to change the base?

v2.2.0 #71

Uh oh!

Conversation

moonglum commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FND left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moonglum commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moonglum commented Mar 9, 2025

Uh oh!

moonglum commented Mar 9, 2025

Uh oh!

moonglum Mar 15, 2025

Choose a reason for hiding this comment

Uh oh!

FND Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

moonglum Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

FND left a comment

Choose a reason for hiding this comment

Uh oh!

FND Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FND Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FND Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

moonglum Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

FND May 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moonglum commented Apr 23, 2025

Uh oh!

moonglum commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moonglum commented Mar 8, 2025 •

edited

Loading

moonglum commented Mar 9, 2025 •

edited

Loading

FND Mar 16, 2025 •

edited

Loading