CDK Construct - Wrong s3 paths generated when deploying from windows

[kostenickj]

Describe the bug

A backslash instead of a forward slash is generated on the static folder, as show below

Actual behavior

this causes CloudFront (and pretty much everything) to fail on everything in that folder and get 403's

Expected behavior

slash should point the right way, like this:
_next/static/

Steps to reproduce

Deploy with cdk construct on windows.
You can see below im using building from a different directory, although i dont know if that makes a difference.

Screenshots/Code/Logs

my builder:

Versions

packages:

• OS/Environment: windows 10
• Next.js version: 10

Additional context

I am going to test on linux and see if it still happens.
By the way, renaming the folder in s3 to the correct structure fixes literally everything.
This error does NOT happen when using the serverless component version, only cdk construct.

Checklist

[dphang]

Probably this line needs to be changed to path.posix.relative?

serverless-next.js/packages/serverless-components/nextjs-cdk-construct/src/index.ts

Line 368 in 4c80140

destinationKeyPrefix: path.relative(assetsDirectory, assetPath),

Can you try to update this and see if it works?

[kostenickj]

I made that change and it failed to deploy on creation of the deployment bucket. I changed it back and it deployed fine, but with the same path error as before

[dphang]

I see, basically I think just some places need to use the posix path which use "/" separator, maybe it is in another line. @kirkness would probably know better since he originally wrote this?

[kirkness]

Hi @kostenickj, you say it failed to deploy when you made that fix, what was the error you saw in the logs?

[kostenickj]

Here is where the initial failure happens:

Also, I was able to confirm my project deploys fine on linux, so definitely a windows thing.

[kirkness]

It's likely a simple fix, although without access to a windows machine myself it'll be a little tricky! Is the above error occurring after setting destinationKeyPrefix to path.posix.relative(...)? It looks as if its using win32 still as the formed command is copying files from a posix path to win32?

[kostenickj]

Yes, i changed it to look like this:

[kirkness]

Cheers for clarifying @kostenickj. From what I can tell, it looks as if the trouble comes from the sources value (as well as the destinationKeyPrefix). The readAssetsDirectory returns values/paths according to the OS, however it appears those are carried through when running on distributions custom resource (based on what I can see in the error message). I'm wondering whether this could be resolved by setting each of the paths returned by readAssetsDirectory using path.posix.join()? Are you able to test this?

[kostenickj]

@kirkness Ill give this a go today, and let you know!

[dphang]

I made a possible fix, looks like invalidation paths also need to use posix path: #1025. Though I also don't have windows machine I can use to test, hopefully it works.

On a related note, we should probably also add at least an e2e test for windows and/or CDK so it doesn't break in the future

[kostenickj]

Didn't have a chance to test yesterday, but i will give it a go today.

[kirkness]

On a related note, we should probably also add at least an e2e test for windows and/or CDK so it doesn't break in the future

Agreed on this - e2e tests would be great. Not sure how the current e2e are set up but it'd be nice to have the same setup across both.

[kostenickj]

I am unable to get this to build correctly, so i will wait until a new alpha release before i can test further.

[dphang]

Have published it, let me know if it works.

@kirkness

Right now e2e tests work by just using a deployment script to manually execute serverless command, run cypress tests and then cleanup all build artifacts, you might be able to create a similar script for CDK deployment and run it as well. Maybe to avoid duplication it can be in next-app package and just run under a different app name to avoid conflicts. But that can be done later