-
Notifications
You must be signed in to change notification settings - Fork 27.3k
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
Support locating non-page js/jsx files with page files #3728
Comments
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:
This would be more maintainable at scale, since we don't duplicate the (root->(chat, timeline)) structure for each file type:
|
@timneutkens Why would you not endorse this practice? |
I see two solutions:
|
@timneutkens @sergiodxa Would you be open to a PR for either of the above solutions? Or perhaps a different solution? |
This really is a deal breaker for people designing large more enterprise applications. Some notable solutions that allow similar functionality: If I had the community support, I would consider making my fork something more modularized & flexible then pushing it to npm something like flext. |
You could actually try to do this:
This gives more granular control over routes that are accessible to the outside world. And is safer and clearer than providing exclude/includes.
To be clear, pagesGlobPattern was always broken. It only accounted for
We're continuously adding features/improvements to face bigger scale, as our own app grows (currently over a thousand components), and as the community gives feedback. |
@timneutkens That's clever, I will give it a shot. Hopefully, the windows guys on my team don't hate me for committing a symlink if it works! This being said, I would prefer to see a roadmap or a consensus from the maintainers on pivotal features. |
@moaxaca the only reason we're hesitant to support the glob is precisely because we are concerned that at scale it could open critical security concerns. We basically would be encouraging people to put into For example, a side effect of not excluding I don't consider it "enterprise ready" to go back on what's otherwise a very clear and secure pattern, but we're always listening to the community for different approaches, in case one is suggested here. In fact, it suffices to do some light research into the vast literature of PHP vulnerabilities to find many that fall under "I exposed a .php file to the end user that was not meant to be world-accessible". |
@timneutkens I am a Windows guy (don't judge) so the symlink approach will not work for me, and besides, there should be a portable solution. But thank you very much for the suggestion! |
What about the second solution that I proposed above?
It seems that this would be less error-prone; users would not have to carefully exclude the files (i.e. write a glob pattern), they just have to make sure that they don't misspell This would also be a very clear pattern I think, no? |
If this is still too error-prone, maybe we could still work with this idea, and extend it with something like "print a warning if a dir like |
I worked around this issue as follows: Think of Create import Home from '../containers/Home'
export default Home This allows for any complex folder structure in |
Beautiful solution @gihrig! 🎉 This solves the problem! Though I still would prefer not to have to write modules like: import Foo from '../../../../../path/to/deeply/nested/container/Foo'
export default Foo and if it was approved I would still make a PR implementing what I proposed above. |
By the way, I also think we do need a solution to not having to write so many import "containers/…" That's also generally useful throughout the codebase, so maybe @gihrig's solution has to be extended with some babel plugin in case you have too many nested paths. I believe @hanford is using such a plugin successfully already. tl;DR: @gihrig's solution is sufficient. It can be combined with the "root" import solution which is orthogonal and generally useful beyond |
@rauchg @zenflow - We're using babel-plugin-module-resolver at Eaze, and I'm a huge fan |
In the same way that writing so many // pages/some/deeply/nested/Foo.js
import Foo from "containers/some/deeply/nested/Foo"
export default Foo note the repetition of "some/deeply/nested/Foo" Also
@rauchg Why not make some particular folder names ignored, as I proposed? Even if it is only a single folder name (like |
@hanford Can you confirm # 1 in my above comment? edit: I see here that with configuration the code completion in the popular IDEs can still work with babel-plugin-module-resolver https://github.com/tleunen/babel-plugin-module-resolver#editors-autocompletion |
import Foo from '../../../../../path/to/deeply/nested/container/Foo'
export default Foo Add import Foo from 'containers/Foo'
export default Foo
...
"plugins": [
...
[
"module-resolver",
{
"root": ["./"],
"alias": {
"components": "./src/components",
"containers": "./src/containers",
"graphql": "./src/graphql",
"lib": "./src/lib",
"pages": "./src/pages",
"src": "./src"
}
}
]
] I keep my 'source' under @hanford beat me by a few, but the details may still be helpful... 😄 |
@zenflow I'm working with VS Code: Re: 1. I have not noticed any problems. I have a pretty strict ESLint setup an it has always told me when I have an unresolved module. Re: 2. I avoid module nesting more that two level below And yeah, it would be super awesome if Next could provide a way to avoid the need for the boilerplate in |
Sorry @gihrig could you clarify your comment about # 2? I'm not sure I understand what you mean, or if I do I don't see how it relates to that issue. |
@zenflow Yeah, I was commenting on problems with deeply nested folders (I try to avoid that). On closer reading, I see you were referring to an 'absolute' import being expected to be under That's a valid point, but since |
@rauchg that makes sense and I completely understand how this could be a loaded gun handed to the masses. I have a ton of empathy for library maintainers that protect their users from themselves. But, (and there's always a butt) the current implementation is very similar to page controller pattern (Talk about some ancient PHP!), and assuming stick with the file system delegating routes, issues like this will always arise. This being said, A NextServer Router that allows route definitions could be optimal. I currently use a Koa instance to handle all this dynamic routing, and while its excellent on the initial hit, it quickly falls apart on the frontend router. I relish the idea of having a router could resolve against my koa definitions while allowing me to structure my app in a modular fashion. Also just to mention, we use Lerna and would love to package pieces our interface in different bundles. A router that could be composed would be amazing. We currently use a similar pattern to the one mentioned above where we import container then just manually link them to root pages. Also if you guys ever need help. I would be happy to contribute some of these features. |
@gihrig Yeah, it's just a minor issue. # 1 is a minor issue as well, since we will not be working in these "link files" often, but I think it's still an issue, since people use a variety of IDE's, each with it's own way of supporting code completion with babel-plugin-module-resolver. |
@moaxaca Let's try to keep this issue concentrated on "Support locating non-page js/jsx files with page files".. I might suggest opening a separate issue for "file-system based routing is not flexible". (btw I think the resolution of issue #257 might help with your modularization issue since you will be able to run multiple apps with common dependencies on the same domain) |
@zenflow Ok, but I am going to leave my comment. I genuinely feel the root of this issue is in the routing mechanics. Also ty for the PR I didn't consider multiple apps 👍. |
The only downside I see with my proposal is that you can no longer have routes containing @moaxaca @gihrig @rauchg Do you see any other downsides or problems? |
@zenflow I agree with @timneutkens I feel like this may create a false sense of security and be prone to exposing too much accidentally. If I were to take a convention over configuration approach, I would prefer a solution where I explicitly define my pages through a .pages.js|jsx|ts extension. They share a common naming convention and don't require blacklisted words like |
@zenflow I don't think we don't need any huge customization for what you wanted. Try using the following approach:
|
@moaxaca Do you think people would be prone to misspelling btw it was @rauchg that said that and I believe (I could be wrong) he was talking only about the I would personally be OK with a |
Just want to add to solution with Instead of
|
@rauchg I don't see this request (it's not an issue) going away any time soon. While I largely agree as to the security related issues, I would argue that if Next is smart enough to look for "pages" in root or "src/pages" and it's smart enough to discriminate or ignore other files/dirs it stands to reason this could be solved with a convention, even if a bit opinionated. While there are workarounds, I'll be honest this really does drive me bonkers at times ;) |
Thank you for your suggestion @gihrig My current go to solution is very simple and becomes very nice to work with, this is ofc a matter of personal taste but instead of creating a folder called // files in pages just render a component from the `src` folder and are basically empty/very think except for data fetching (which can also be moved into `src` folder if you want to)
pages\...
src\common\...
src\start\...
src\profile\...
src\contact\... It might feel odd to see the pages folder outside the |
In addition to the pageExtensions idea (which would be awesome), you could allow optionally setting a "page component prefix" in next.config.js that would cause next to ignore any files with this prefix. Say you set it to "_", then you could have :
Kind of like the ESLint no-unused-vars prefixIgnorePattern option. |
Seems like using Custom Page Extensions is a good solution. See: |
Does the global css load for you if using this approach? my setup is: // pages/items/new
import ModulesNewItem from "@modules/items/new";
export default function newItem({ ...props }) {
return <ModulesNewItem {...props} />;
}
OR
export default ModuleNewItem
// src/modules/items/new
export default function ModulesNewItem() {return(<>...content</>)} |
Citing possible user error as a reason not to implement filters on page files is not well founded. I guarantee there are nextjs sites out there that have test and supporting files in |
Credit to stackoverflow, I think this is the clearest option to maintain the
We're going for principle of least surprise, right? Make the pages folder permit the normal test code convention. 🤷 This doesn't solve the general "but i really want to" reasoning for putting other components in the pages directory. I guess do this if you want actively break conventions instead of following them. Edit: actual credit to this undescribed code snippet from the config docs. 😂 |
The solution that @sethbattin suggested is great, it worked for me using webpack 4. However, I am using webpack 5 in my Next.js projects. To adapt the solution to work with webpack 5, I tried the following configuration (Next.js 10.2.3): module.exports = {
future: {
webpack5: true,
},
webpack: (config, { webpack }) => {
config.plugins.push(
new webpack.IgnorePlugin({ resourceRegExp: /\/__test__\// })
);
return config;
},
}; See https://webpack.js.org/plugins/ignore-plugin for details on this change. Essentially, the webpack team changed the implicit regex parameters to now be passed within an object. Unfortunately, builds error out with |
I started collocating tests to the Just feedback to the other ideas, the page extension solution looks plausible for new projects, and I would do this in the future, but changing the extensions on hundreds of pages on an existing project is not reasonable. Not sure why an include/exclude or other pattern matcher options are not supported. |
…ng in /pages. Note - if you need tests in /pages there is a way. vercel/next.js#3728 (comment)
It's working well in |
@qafoori - thank you for the suggestion! I just tried it and all my pages gave me 404s, though 🙁. If anyone has a guess as to why, I'd love to hear it! I'd much prefer to leave my components/tests/pages colocated, but for now I've gone for the "two folders" solution with one of those folders called |
The two folder is not the best practice at all. |
I solved this for our project by using Custom Page Extensions.
module.exports = {
// Force .page prefix on page files (ex. index.page.tsx) so generated files can be included in /pages directory without Next.js throwing build errors
pageExtensions: ['page.tsx', 'page.ts', 'page.jsx', 'page.js'],
} It seems like this has been mentioned a few times but this issue is still filling up my inbox with new users trying to figure it out 😄 My initial discovery of how to do this in a comment above: #3728 (comment) If this works for you, maybe you can upvote the docs PR or suggest things that would make this solution more discoverable. |
It worked! Sorry I don't/didn't have more details about what went wrong when I tried a few days ago - i was at the end of lots and lots of reading the relevant GitHub issues about it, and was just trying to get something to work as quickly as possible. Thanks Scotty / @OzzieOrca - and thanks again @qafoori Also, for what it's worth, I actually think this solution is better (explicit opt-in) than an explicit opt-out solution that others have advocated for. Much less room for user error and it doesn't really prevent you from any dev workflows. Hey, maybe even the default should be -Scotty |
This is a great suggestion, but it still introduces a new rule that others will have to follow if you have this enabled in your project. I still vote for the addition of an ignore pattern option in |
I also ran into this same error trying to use |
…omponents (#22740) Feel free to edit this/suggest changes as you see fit to match the style of your docs or how you want this feature represented. There seemed to be many issues regarding colocating non-page files with page components in the `pages` directory where `pageExtensions` was finally suggested as a solution. I think it would be great to document it better since it seems to be a often requested workflow to colocate test, generated files, styles, and other files needed by page components in the `pages` directory. The note that exists in the docs currently alludes to the ability to do this but it doesn't contain much context that would help make it more discoverable. Relevant issues: #11113 (reply in thread) #8454 (comment) #8617 (comment) #3728 (comment)
Is there a way to pass styles to src/pages ? It doesn't work with TailwindCSS v3 if pages is within src on nextjs v12. But works if placed within root |
This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you. |
Original title: Support having non-page js/jsx files in pages dir
This is an often requested feature. See issues #3508 #1914 #1689 #1545 #988 and there are likely more. edit: Also #3183.
This issue was fixed in PR #3195 but somewhere along the way the
pagesGlobPattern
config became broken, and was cleaned out in PR #3578.This feature would allow us to place files that support specific pages, with the pages that they support. These "support" files could be tests, components, utilities, and more. This would improve the application's file organization, especially for large projects.
The text was updated successfully, but these errors were encountered: