Skip to content

refactor: drop walk and minimatch#959

Merged
willrowe merged 6 commits into
twigjs:masterfrom
slagiewka:drop_walk_minimatch
May 12, 2026
Merged

refactor: drop walk and minimatch#959
willrowe merged 6 commits into
twigjs:masterfrom
slagiewka:drop_walk_minimatch

Conversation

@slagiewka
Copy link
Copy Markdown
Contributor

This commit drops the combined use of walk and minimatch by using fs.readdir and path.matchesGlob.

The baseline for this commit is node v22.5.

Closes #955
Closes #956

PS. I run this on 22.4 to confirm it faling. Then it turns out that you can't run the test suite without NODE_OPTIONS="--experimental-require-module" due to chokidar being ESM-only.

Comment thread lib/compile.js
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove async and promises and just get the paths synchronously like it did before.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Previous version was not synchronous. Walk emitted events and the main compile export function wouldn't really wait for it.
What's more, even if I make my changes use readdirSync + the main compile func statSync it's still not synchronous due to twig() constructor not calling async: false.

I've made it full sync now by combining the above.

PS. It looks to me like the recursive flags from the options (defaults to false) was and still is a no-op. We could technically now make it work as expected with readdirSync params.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good points, I didn't notice that before. Thank you for getting it to work synchronously all the same. It makes it easier to work with in its current state. The package is a weird mix of async and sync that needs to get converted to all async, but until then this is a big help.

@slagiewka slagiewka force-pushed the drop_walk_minimatch branch from 0e3071d to dba836e Compare May 12, 2026 07:04
@slagiewka slagiewka requested a review from willrowe May 12, 2026 07:58
@willrowe willrowe force-pushed the drop_walk_minimatch branch from d7f1c26 to 90722b2 Compare May 12, 2026 12:31
Comment thread lib/compile.js
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good points, I didn't notice that before. Thank you for getting it to work synchronously all the same. It makes it easier to work with in its current state. The package is a weird mix of async and sync that needs to get converted to all async, but until then this is a big help.

@willrowe
Copy link
Copy Markdown
Collaborator

@slagiewka thank you so much for your work on this. I made a couple of last tweaks to clean up a couple of things.

slagiewka and others added 6 commits May 12, 2026 08:52
This commit drops the combined use of walk and minimatch by using
fs.readdir and path.matchesGlob.

The baseline for this commit is node v22.5.
- `readdirSync` does not have a callback argument.
- This was leftover from the CLI version of the package before automatic directory walking was added.
@willrowe willrowe force-pushed the drop_walk_minimatch branch from adfe7d3 to ab24fc3 Compare May 12, 2026 12:52
@willrowe willrowe merged commit 3d2b08f into twigjs:master May 12, 2026
3 checks passed
@slagiewka slagiewka deleted the drop_walk_minimatch branch May 12, 2026 15:23
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.

2 participants