Add an option for createStaticAssets to respect the basePath of Next.js #821
Conversation
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
commit: |
There was a problem hiding this comment.
If we do that we'll need to modify this file as well otherwise it will break existing aws deployment https://github.com/opennextjs/opennextjs-aws/blob/main/packages/open-next/src/build/generateOutput.ts
I'm wondering if the good long term solution would be to actually use that output file instead ?
Yes, it's more a short term hack for cloudflare... |
There was a problem hiding this comment.
Yeah it is used in sst and other places, but i've missed the useBasePath which should make it do nothing there.
FYI every places that use this function need to use the basePath (this includes public file)
|
I'm not quite sure what the best way to move forward here is? |
|
I'm not 100% sure either, my first idea would be that cloudflare (or anyone else) should use the output file to actually finalize the deployment from the created assets from OpenNext. For example these 2 lines: opennextjs-aws/packages/open-next/src/build/generateOutput.ts Lines 201 to 202 in 73281c9 createStaticAssets because files would be at .open-next/assets/basePath and would thus create an _assets/basePath/basePath folder.
|
The thing I don't get with this code is that static assets do not seem to be prefixed by the basePath? |
|
On aws it's on the behavior for cloudfront, not in the bucket themselves |
| fs.cpSync( | ||
| path.join(appBuildOutputPath, ".next/static"), | ||
| path.join(outputPath, "_next", "static"), | ||
| path.join(outputPath, basePath, "_next", "static"), | ||
| { recursive: true }, | ||
| ); | ||
| if (fs.existsSync(appPublicPath)) { |
There was a problem hiding this comment.
Everything in here should be under outputPath/basePath
There was a problem hiding this comment.
What do you mean by "everything in here"?
Looking at the blog example, path are not re-written. The avatar will not be fetch for the basePath. Only <Image/> and <Link/> will use the base path.
There was a problem hiding this comment.
Favicon and assets in public should also start with basePath. That's the behavior in next start
There was a problem hiding this comment.
That's not the behavior I observe on the blog example with next start?
There was a problem hiding this comment.
If i add the /blog basePath that's what i get for the blog example
There was a problem hiding this comment.
Oops, you'right.
What I did with the blog example is that I probably used npm preview and make that work.
As the path are nor rewritten, the assets should not be prefixed if you want to access them.
But it must be an overlook in the blog example.
In conclusion, you are right, everything must be prefixed.
Would that work if the copy is unconditionally done to the basePath subDir and the basePath logic is removed from the output file? I think this could be a way to support Cloudflare & AWS?
There was a problem hiding this comment.
We need to add it to the behavior as well anyway (to avoid intercepting things it should not).
I think we go for your solution for now, just add a comment about it, and try to figure out a way to create an output that could be used by everyone.
I had this idea awhile back to add an outputOverride to be able to compute an output that could be used in each platform easily. This inside something like an open-next.build.ts could be something that could help us in a lot of ways.
|
I updated the PR to add a comment, does it looks good like this? |
|
Thanks Nico! |
The plan is to use this in cloudflare so that the path it handled by the Workers Assets automatically when a basePath is configured