Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Next JS: automatically copy public folder on running nx build myApp #3019

Closed
asherccohen opened this issue May 16, 2020 · 16 comments · Fixed by #3047
Closed

Next JS: automatically copy public folder on running nx build myApp #3019

asherccohen opened this issue May 16, 2020 · 16 comments · Fixed by #3047
Assignees
Labels
outdated scope: nextjs Issues related to NextJS support for Nx type: bug

Comments

@asherccohen
Copy link

asherccohen commented May 16, 2020

  • [x ] I am running the latest version
  • [x ] I checked the documentation (nx.dev) and found no answer
  • [x ] I checked to make sure that this issue has not already been filed
  • [x ] I'm reporting the issue to the correct repository (not related to React, Angular or any dependency)

Expected Behavior

Upon running nx build myApp for a next js project all public folder should be copied to target dist folder. Nx 9.3 has made changes to the builder but didn't introduce this and we're forced to do it manually.

Would also be super cool (but unsure how could this work) to be able to populate package.json with proper dependencies from the specific project. Is there a way to check what deps the projects needs and copy these from the root package.json? This is not a blocker if it's impossible.

Current Behavior

Public folder is not copied and we have to remember to do it manually which can be a barrier for new starters.

Failure Information (for bugs)

@jaysoo can help with this as he has implemented the builder.

Steps to Reproduce

  1. generate a next js project
  2. add some static files to public folder
  3. run 'nx build myNextJsApp'
  4. check the dist folder for that app, you'll notice no public folder has been copied
  5. try to deploy and your static assets will be missing
@dai4
Copy link

dai4 commented May 16, 2020

+1 for both points you've raised. There's no much point of an automatically generated package.json if it doesn't contain all the dependencies of the project that's been built. Ideally, a vendor chunk should be generated so then all it's required is to just start next and not install anything else. Especially useful for artifact deployment.

@asherccohen
Copy link
Author

asherccohen commented May 17, 2020

Thanks. I would add that I've tried generating a brand new next js app and noticed that the "public" folder is not called "public" at all, it's named "assets".

For consistency I would suggest naming it "public" as explained here https://nextjs.org/docs/basic-features/static-file-serving.

Also (I know I'm being very very picky here but I feel this point is also important) I have tested deploying to zeit vercel (the official next js hosting platform) and must confirm that it doesn't work.

The reason is that now cli ignores the local .next folder on commit. It's designed to build on the cloud but we know we can't do that due to our shared lib structure in nx (and we're not going to push all of the monorepo to zeit right?we want to just push the dist/apps/myApp built artifacts).
A .vercelignore file can bypass this as explained here https://vercel.com/guides/prevent-uploading-sourcepaths-with-nowignore but I'm still struggling to make it run on their platform. Can anyone suggest how?

I've raised this question here to them: https://twitter.com/code_tank_dev/status/1261752786213101570?s=19

So to recap:

  • name assets to public
  • move public to dist folder
  • investigate how to deploy to zeit vercel

This would make support super strong!

@FrozenPandaz FrozenPandaz added the scope: nextjs Issues related to NextJS support for Nx label May 19, 2020
@benediktvaldez
Copy link
Contributor

This PR implemented generating a package.json, but it doesn't add any of the production dependencies in the root package.json, it only adds next, react and react-dom which is fine for a basic app but doesn't do much good as soon as you add any other dependencies needed at runtime.

Identifying what packages are actually being required within the app (which I don't think is that far out there compared to some of the magic the nx tool is capable of?) would be the perfect solution, picking up the version from the root package.json.

@benediktvaldez
Copy link
Contributor

I do believe assets and public have different purposes so keeping both might be the way to go. The convention here seems to be that assets are to be imported to be used, while public is used by next as a static folder that is served as is. Just a thought?

@rarmatei
Copy link
Collaborator

rarmatei commented May 20, 2020

This PR implemented generating a package.json, but it doesn't add any of the production dependencies in the root package.json, it only adds next, react and react-dom which is fine for a basic app but doesn't do much good as soon as you add any other dependencies needed at runtime.

Identifying what packages are actually being required within the app (which I don't think is that far out there compared to some of the magic the nx tool is capable of?) would be the perfect solution, picking up the version from the root package.json.

The reason next, react and react-dom are added is that they're needed to run next start in your dist/my-next-app folder. Regarding other dependencies - would they not be bundled into your built app, and hence not need to be included/installed again through the copied package.json?

@dai4
Copy link

dai4 commented May 20, 2020

As far as I could see, the next builder doesn’t package the vendor libraries separately but rather expecting them on start. Is there a way to tell Next to create a separate chunk with dependencies?

@benediktvaldez
Copy link
Contributor

The reason next, react and react-dom are added is that they're needed to run next start in your dist/my-next-app folder. Regarding other dependencies - would they not be bundled into your built app, and hence not need to be included/installed again through the copied package.json?

I don't think next does it like that, since it seems to assume the packages will be available in node_modules when you run next start. At least my app wouldn't run without some of these packages. Maybe I'm doing something wrong 🤷🏻‍♂️ It might have something to do with what kind of an app you have?

@asherccohen
Copy link
Author

Here is where things get tricky. Not all of those packages are needed to run your next project from dist/myApp. At least not the ones needed at build time, it's a bit like the concept behind dependencies and devDependencies, aka what is needed to build and what to run.

We also need to be careful with two things:

  • shared libs (so packages that are not directly taken from node_modules) although I think these have been added at build time.

-what happens when u try to upload to Zeit Now (I have a separate issue open for this), since the built .next folder cannot be committed. Zeit Now requires to build on their platform to be able to take advantage of all next features (api routes/dynamic routes etc)

@rarmatei
Copy link
Collaborator

rarmatei commented May 21, 2020

The reason next, react and react-dom are added is that they're needed to run next start in your dist/my-next-app folder. Regarding other dependencies - would they not be bundled into your built app, and hence not need to be included/installed again through the copied package.json?

I don't think next does it like that, since it seems to assume the packages will be available in node_modules when you run next start. At least my app wouldn't run without some of these packages. Maybe I'm doing something wrong 🤷🏻‍♂️ It might have something to do with what kind of an app you have?

That's very interesting if that's the case. I don't have that much experience with Next.js, but I tried bootstrapping a new Next.js app locally then..

  1. added a date-fns dependency and used some of its functions in the app
  2. did a next build
  3. completely recreated node_modules without date-fns (I deleted date-fns from package.json then rm -rf node_modules && yarn
  4. did a next start - and the app worked just fine - because date-fns had been bundled to the dist folder as part of step 2.
  5. if we remove react, react-dom or next as dependencies, next start rightfully fails

There seem to be 2 separate discussions here:

  1. copying the public folder to dist
  2. copying app dependencies over to the package.json in the dist folder

Let's keep this issue focused on 1. please - copying the public folder to dist.


If we think 2. is a problem, can you please create another issue with a repo with an example next.js app (doesn't have to be an Nx workspace), similar to my steps above, but where step 4. fails?
Or is there any other reason I missed on why you think that's needed?

@asherccohen
Copy link
Author

I agree...numver 1 (copy public folder) is the priority.

I'll also work on a test project and create a separate issue. Tx.

@jaysoo
Copy link
Member

jaysoo commented May 21, 2020

Hi @asherccohen, do you have a repo where we can see the bug?

I created an app that display a single jpeg, and the assets seem to load fine with both static and ssr server.

https://github.com/jaysoo/nx-next-assets

@asherccohen
Copy link
Author

Oh!I might know why this is happening without sending you a repo. You are naming the "public" folder as "assets" so I guess your builder will try to copy this folder instead of "public".

Naming convention in next.js apps for the static/assets/public folder is "public".

@jaysoo
Copy link
Member

jaysoo commented May 21, 2020

Ah, I got it. Thanks for the clarification!

@jaysoo
Copy link
Member

jaysoo commented May 21, 2020

PR for the fix is open, should be in the next patch release.

@asherccohen
Copy link
Author

Superb!thank you so much!

@github-actions
Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated scope: nextjs Issues related to NextJS support for Nx type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants