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

The build assumes that all js files under /pages is a page #3183

Closed
emattias opened this issue Oct 26, 2017 · 14 comments · Fixed by #3195
Closed

The build assumes that all js files under /pages is a page #3183

emattias opened this issue Oct 26, 2017 · 14 comments · Fixed by #3195

Comments

@emattias
Copy link
Contributor

emattias commented Oct 26, 2017

We have structured our files so that page specific code is in the pages folder. Here is an example:

  • pages
    • manage-users
      • __tests__
        • manage-users.test.js
      • index.js
      • components
        • ManageUsersPage.js
        • <other page specific components here>

The index.js file has the ManageUsersPage.js components as its default export. That is a regular next.js page component

This structure has worked fine for us except when we build. This hardcoded glob assumes that all js files under pages is a page: https://github.com/zeit/next.js/blob/canary/server/build/webpack.js#L50

Does it make sense to make this glob overridable or do we have to not use file based routing?

@liweinan0423
Copy link
Contributor

liweinan0423 commented Oct 26, 2017

I think you can move non-page components to another top-level folder and only keep page file under pages folder.

@emattias
Copy link
Contributor Author

emattias commented Oct 26, 2017

Yes, but I think having components (and other js files) under their respective /pages/<page name> folder is good because it makes it clear that the components are specifically for that page.

@liweinan0423
Copy link
Contributor

liweinan0423 commented Oct 26, 2017

@emattias This is the limitation that next.js currently has. but I think we can alway work around it.

what about the following structure?

- pages
   - page1.js
   - page2.js
- components
   - page1
      - page-1-component-1.js
      - page-1-component-2.js
   - page2
      - page2-component.js

also I think Convention Over Configuration is the one of the goals of next.js

@zenflow
Copy link
Contributor

zenflow commented Oct 26, 2017

#1914 is related. It's marked closed but it's still active.

@zenflow
Copy link
Contributor

zenflow commented Oct 26, 2017

Yup, this issue prevents us from using a folders-by-feature organization, which would be more scalable than the folders-by-type organization that is being imposed on us

@sergiodxa
Copy link
Contributor

sergiodxa commented Oct 26, 2017

@zenflow I think a page is not always a feature, I mean a single page could have more than one feature of your app at the same time.

e.g. pages/index.js could be showing the feature Chat and the feature Timeline (like FB) in the same page.

So pages aren't features. You could use something like:

/features
  /chat
  /timeline
  /another-feature
/pages
  /index.js
  /something.js

@emattias
Copy link
Contributor Author

emattias commented Oct 27, 2017

@sergiodxa I agree they aren't always but when you have components that are only used in that page it made it clearer with the structure I described initially.

What about making the glob overridable?

@zenflow
Copy link
Contributor

zenflow commented Oct 27, 2017

@emattias Configurable pagesGlobPattern would resolve this issue (thanks for submitting the PR) but here's another proposal, which might be better considering Next's convention-over-configuration philosophy:

Make all lib/, __tests__/, and __mocks__/ folders ignored. (edit: __test__/ and __mock__/, singular, maybe too, since i think they may be as conventional as their plural counterparts) No configuration necessary, and it fits all use cases we've come across.

The idea of lib/ folders is that they contain supporting code for their parent folder, so your components (along with helper functions and whatever else) would go somewhere in there.

@arunoda Could we get your thoughts?

@zenflow
Copy link
Contributor

zenflow commented Oct 27, 2017

For clarification

This is the folder structure we want to avoid, especially if it is to scale to have more than 3 pages in a hierarchy more than 1 deep:

  • components/
    • chat/
    • timeline/
    • (... index components)
  • helpers/
    • chat/
    • timeline/
    • (... index helpers)
  • pages/
    • chat.js
    • timeline.js
    • index.js
  • tests/
    • chat/
    • timeline/
      (... index tests)

This would be more maintainable at scale, since we don't duplicate the (root->(chat, timeline)) structure for each file type:

  • pages/
    • __tests__/
    • lib/
      • components/
      • helpers/
    • chat/
      • __tests__/
      • lib/
        • components/
        • helpers/
      • index.js
    • timeline/
      • __tests__/
      • lib/
        • components/
        • helpers/
      • index.js
    • index.js

(edit: components/ and helpers/ can and should have their own __tests__/)

@zenflow
Copy link
Contributor

zenflow commented Oct 27, 2017

@emattias Would my proposal fit your use case?

@emattias
Copy link
Contributor Author

emattias commented Oct 29, 2017

@zenflow I dont want to have to structure pages folders differently than other domains in our project. There there is no need for a lib/ sub folder for example.

What if instead of excluding everything that is not a page, we have a convention where you can help the build script identify pages. For example with a filename suffix of .page.js
Then you could structure things however you want/need.

@zenflow
Copy link
Contributor

zenflow commented Nov 3, 2017

@emattias

Files inside of pages/ intuitively seem to have already opted into being a page, by virtue of being under pages/. I think it makes more sense to opt back out via a lib/ sub folder, rather than opt in a second time via a .page.js suffix.

Also consider that .page.js would be a breaking change affecting every project using Next.js. Many/most users have no problem with the current folders-by-type layout, and IMO it works fine for small projects. We should try to avoid imposing any changes on those users and projects, especially if it makes things less ergonomic for them, just to make things more ergonomic for us.

What is the harm in structuring the pages folders differently than other domains? Perhaps you could bring this convention to those other domains? E.g. if you have a models/ folder and some of the files under it are not models, explicitly indicate this by putting them under a lib/ sub folder.

Do you think my proposal could satisfy your use case?

@zenflow
Copy link
Contributor

zenflow commented Nov 3, 2017

@liweinan0423 Would love to hear your thoughts on this!

@lock
Copy link

lock bot commented May 10, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants