Skip to content

chore: replace yazl with fflate#148

Open
X-Guardian wants to merge 2 commits intoopen-constructs:mainfrom
X-Guardian:fix/replace-yazl-with-fflate
Open

chore: replace yazl with fflate#148
X-Guardian wants to merge 2 commits intoopen-constructs:mainfrom
X-Guardian:fix/replace-yazl-with-fflate

Conversation

@X-Guardian
Copy link
Copy Markdown
Contributor

@X-Guardian X-Guardian commented May 2, 2026

Description

The archiveSync function in packages/cdktn/lib/private/fs.ts runs from TerraformAsset._onSynthesize, which executes inside the synchronous constructs synth lifecycle, so it has to block until the zip is on disk. yazl only exposes a streaming API, so the previous implementation worked around this by re-invoking the current module file as a child Node process via execSync(node ${__filename} ...). That hack relies on __filename always resolving to a CommonJS-loadable JS file — an assumption that breaks the moment ts-jest (or anything else) causes __filename to point at a .ts source. Node's strip-types loader applies ESM resolution to such files and rejects the extensionless relative import from "../errors".

fflate ships a real synchronous zip API (zipSync) and, for the test rig's HTTP-streaming use case, a streaming Zip / ZipDeflate pair. Swapping yazl for fflate lets us delete the child-process bridge entirely and the whole class of layout-fragility bugs that come with it.

  • archiveSync rewritten on fflate.zipSync — ~15 lines, no subprocess, no __filename dependency, no inline runner script.
  • test/template-server.ts migrated to fflate.Zip + ZipDeflate; chunk-by-chunk streaming to the HTTP response is preserved.
  • yazl + @types/yazl removed from packages/cdktn and test; fflate added to both. Root nohoist updated so fflate lands inside packages/cdktn/node_modules/ for bundledDependencies.

Checklist

  • I have updated the PR title to match CDKTN's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

@X-Guardian X-Guardian marked this pull request as ready for review May 3, 2026 08:26
@X-Guardian X-Guardian requested a review from a team as a code owner May 3, 2026 08:26
Copy link
Copy Markdown
Contributor

@jsteinich jsteinich left a comment

Choose a reason for hiding this comment

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

I do like the simplifications made, but I am a bit concerned with using fflate. It hasn't been updated in 2 years which isn't necessarily a deal breaker; however, there are some reported potential compatibility issues with Node 22 and TypeScript 5.9.

@so0k
Copy link
Copy Markdown
Contributor

so0k commented May 4, 2026

It hasn't been updated in 2 years

Owner is responding to discussion - 101arrowz/fflate#248 (reply in thread)

The Node v22 discussion - 101arrowz/fflate#227

This issue does not affect any synchronous functions because no data needs to be transferred between threads. It also doesn't affect most of the asynchronous functions because they do not assume consume the input by default (i.e. they copy memory to the other thread instead of transferring). However, it does affect the streaming asynchronous APIs, including AsyncZipDeflate and cousins, because those do consume the input (transfer memory instead of copying) by default.

For now, you can work around the issue by copying all buffers before passing them into a streaming asynchronous API: ... <workaround for AsyncZipDeflate>

The metamask project added the workaround, but then replaced fflate with yazl in this commit

MetaMask/metamask-extension@f9e5904#diff-161cb29682beb647abebd2d85da170103ef4db84989977124f1327c9a9b7153e

But they used fflate in a webpack plugin, so moving to yazl makes sense for them and doesn't apply to us?

... the asset-moving and zip-writing pipeline harder to reason about and harder to optimize. ...

I think we can conclude that Nodev22 should work (they had the workaround in place Dec 2024 -> Apr 2026)

@jsteinich
Copy link
Copy Markdown
Contributor

There is also 101arrowz/fflate#242 which could be a concern

@so0k
Copy link
Copy Markdown
Contributor

so0k commented May 6, 2026

perhaps @dnwlf has some insights - I didn't realise yazl was just introduced in #95

@dnwlf
Copy link
Copy Markdown
Contributor

dnwlf commented May 6, 2026

perhaps @dnwlf has some insights - I didn't realise yazl was just introduced in #95

I don't have any concerns that weren't already raised in this discussion. I viewed #95 as a short-term solution to the lodash CVE (a lateral change), with an understanding there was already a desire for longer-term improvements. It's ok with me if my short-term solution is very short-term.

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.

4 participants