Skip to content

Conversation

rmarscher
Copy link
Member

Fixes #944

Copy link

vercel bot commented Nov 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Nov 6, 2024 7:22pm
Copy link

codesandbox-ci bot commented Nov 6, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

const srcPath = path.join(srcDir, file.name);
const destPath = path.join(destDir, file.name);
if (file.isDirectory()) {
mkdirSync(destPath, { recursive: true });
Copy link
Member

Choose a reason for hiding this comment

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

does this recursive option work on node 18? it might have been with another function call but I think we saw that this got added in node 20 for something else, so it seems good to double check.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@rmarscher rmarscher Nov 6, 2024

Choose a reason for hiding this comment

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

I think it is readdir's recursive option that isn't consistent across node versions. I think mkdir recursive is more stable.

Comment on lines 154 to 160
copyFiles(assetsDistDir, workerPublicDir, [
'.txt',
'.html',
'.json',
'.js',
'.css',
]);
Copy link
Member

Choose a reason for hiding this comment

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

would this break if there were files like a CNAME of favicon?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. The purpose of this is to copy built public assets that might imported during SSR.

It would be better if we had a manifest of these files to copy rather than making assumptions by file extension.

@dai-shi dai-shi mentioned this pull request Nov 6, 2024
@dai-shi
Copy link
Member

dai-shi commented Nov 6, 2024

@rmarscher Please check the CI error. It fails on Windows.

@rmarscher
Copy link
Member Author

@rmarscher Please check the CI error. It fails on Windows.

I see. Hmm... maybe it needs to get an OS temp dir.

EPERM: operation not permitted, rename 'D:\a\waku\waku\examples\31_minimal\dist' -> 'D:\a\waku\waku\examples\31_minimal\_dist'
@dai-shi dai-shi merged commit 2037f4d into wakujs:main Nov 7, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants