Skip to content

Fix the module import workaround for Windows #174

Open
segevfiner wants to merge 3 commits into
pinojs:mainfrom
segevfiner:windows-worker-workaround-fix
Open

Fix the module import workaround for Windows #174
segevfiner wants to merge 3 commits into
pinojs:mainfrom
segevfiner:windows-worker-workaround-fix

Conversation

@segevfiner

Copy link
Copy Markdown

Suprisingly we had pkg code actually hit the ERR_MODULE_NOT_FOUND on Node.js 22, but the workaround for Windows was missing there...

BTW it might be better to use require('url').fileURLToPath() for this?

@mcollina

Copy link
Copy Markdown
Member

Can you look at the failing tests?

@segevfiner segevfiner left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This project doesn't use a lock file and the latest version of @yao-pkg/pkg seems to not work with Node.js<=16. Something about not finding fs.promises.access.

@segevfiner

Copy link
Copy Markdown
Author

Hrmm the tests passed locally for me... So now we have some test that's only failing in CI?

@mcollina

Copy link
Copy Markdown
Member

I don't understand, this project CI is 18+.

@segevfiner

segevfiner commented Jul 27, 2025

Copy link
Copy Markdown
Author

The pkg.config.js said to build pkg binaries with older Node.js versions. I updated this in the PR. But now something else is failing only in CI.

@mcollina

mcollina commented Dec 2, 2025

Copy link
Copy Markdown
Member

@segevfiner please rebase this PR.

Suprisingly we had pkg code actually hit the ERR_MODULE_NOT_FOUND on Node.js 22, but the workaround for Windows was missing there...

BTW it might be better to use `require('url').fileURLToPath()` for this?
@segevfiner segevfiner force-pushed the windows-worker-workaround-fix branch from 215e4a4 to d1adeba Compare December 2, 2025 08:29

@segevfiner segevfiner left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Rebased

@mcollina

mcollina commented Dec 2, 2025

Copy link
Copy Markdown
Member

Can you add a test for this?

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina

mcollina commented Dec 2, 2025

Copy link
Copy Markdown
Member

Does that test fails on Windows without this patch?

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