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

Add a Next.js example #27

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Conversation

milos-sikic-nimbus-tech
Copy link
Contributor

Add an example of how you can support Next.js projects with the plugin and a custom pattern.

This pattern should accept kebab-case naming, as well as square brackets around the names, i.e. [locale], [...auth], etc. should work as well.

@milos-sikic-nimbus-tech
Copy link
Contributor Author

milos-sikic-nimbus-tech commented Jun 5, 2023

@dukeluo I just realized that this pattern does not support catch-all routes, i.e. [...auth]. Do you think it would be possible to incorporate that, too?

There are three types of dynamic routes:

Dynamic segments: [slugName]
Catch-all segments: [...auth]
Optional catch-all segments: [[...auth]]

I'm really sorry to be a bother like this, but it seems that it would be better if the names inside of the segments are camelCase, because they translate into parameters that you can use inside your JS code (and kebab-case needs to be escaped there).

Is it possible to make a pattern that matches with camelCase if there are brackets around the name, and kebab-case if there are not? If not, could you also do a variant that does camelCase everywhere? I think it would be better to offer both in the docs and let people choose what they like best.

Reference: https://nextjs.org/docs/app/building-your-application/routing/dynamic-routes

@dukeluo
Copy link
Owner

dukeluo commented Jun 6, 2023

@milos-sikic-nimbus-tech There are three types of dynamic routes:

  • Dynamic segments: [slugName]
  • Catch-all segments: [...auth]
  • Optional catch-all segments: [[...auth]]

And two naming styles:

  • KEBAB_CASE
  • CAMEL_CASE

The glob expressions to achieve this will like this, they look so complicated:

const CAMEL_CASE = '+([a-z])*([a-z0-9])*([A-Z]*([a-z0-9]))';
const KEBAB_CASE = '+([a-z])*([a-z0-9])*(-+([a-z0-9]))';

// these two glob expression are not tested
const NEXTJS_DYNAMIC_ROUTES_WITH_KEBAB_CASE = `@(${KEBAB_CASE}|\\[${KEBAB_CASE}\\]|\\[\\.\\.\\.${KEBAB_CASE}\\]|\\[\\[\\.\\.\\.${KEBAB_CASE}\\]\\])`;
const NEXTJS_DYNAMIC_ROUTES_WITH_CAMEL_CASE = `@(${CAMEL_CASE}|\\[${CAMEL_CASE}\\]|\\[\\.\\.\\.${CAMEL_CASE}\\]|\\[\\[\\.\\.\\.${CAMEL_CASE}\\]\\])`;

I think it will be better if they can be supported by a builtin. I'm not familiar with next.js Dynamic Routes, would you like to help me test these glob expressions?

@milos-sikic-nimbus-tech
Copy link
Contributor Author

Hey @dukeluo, of course! I'll try these out all let you know how they work. Currently a bit swamped by work, but I'll get back to you until the end of the week.

Sorry for the delay!

@milos-sikic-nimbus-tech
Copy link
Contributor Author

Hi @dukeluo, I've been researching the documentation to cover as many cases as possible while testing, and unfortunately, I found out that the problem is even more complex that I initially outlined.

Next JS with the App Router supports multiple differently named constructs. Each of them represents a different thing in the system, i.e. the name changes the functionality.

So it goes something like this:

Standard routes

/src/app/about/page.js -> www.mywebsite.com/about

These should be kebab-case.

Dynamic segments

/src/app/help/[helpPageId]/page.js -> www.mywebsite.com/help/3 (helpPageId is 3 in this case)

These should be camel case.

Catch-all segments

/src/app/[...auth]/page.js -> www.mywebsite.com/auth, www.mywebsite.com/auth/new, www.mywebsite.com/auth/new/2

These should be camel case.

Optional Catch-all Segments

The same as above, but the param is optional:

/src/app/[[...auth]]/page.js -> www.mywebsite.com/auth, www.mywebsite.com/auth/new, www.mywebsite.com/auth/new/2, AND www.mywebsite.com

These should be camel case.

Route groups

Only used for organization, gets skipped in the URL.

/src/app/(auth)/page.js -> www.mywebsite.com

These should be kebab-case.

Named slots

Start with an @, used for rendering multiple pages in one layout

/src/app/@feed/page.js -> www.mywebsite.com
/src/app/@suggestions/page.js -> www.mywebsite.com

These should be kebab-case.


So, with all of that in mind, I think this gets very complex.

Here are some test cases for you:

Should pass:

/src/app/page.ts
/src/app/example-route/page.ts
/src/app/users/[userId]/page.ts
/src/app/[...auth]/route.ts
/src/app/shop/[[...shopId]]/page.ts
/src/app/@auth/page.ts
/src/app/(marketing)/page.ts

Should not pass:

/src/app/somePage.ts
/src/app/exampleRoute/page.ts
/src/app/users/[userId/page.ts
/src/app/users/userId]/page.ts
/src/app/[..auth]/route.ts
/src/app/[...auth/route.ts
/src/app/...auth]/route.ts
/src/app/shop/[[...shopId]/page.ts
/src/app/shop/[...shopId]]/page.ts
/src/app/shop/[[..shopId]]/page.ts
/src/app/@authMarker/page.ts
/src/app/(marketingSpeak)/page.ts

Would it help if I created a repo with all of these examples?

Because of the complexity of the naming rules, I'm kinda leaning towards this actually not being a part of the library, as I feel it would bloat it and make it less agnostic. I'm more for the documentation approach that you have recommended initially, but of course, it's your choice.

Also, I understand that you might not want to support all of these cases, in any case, I am greatly helpful for all the info you provided so far!

Another question, I have made a mixed case pattern from the patterns you provided. Does this seem correct to you?

const CAMEL_CASE = '+([a-z])*([a-z0-9])*([A-Z]*([a-z0-9]))';
const KEBAB_CASE = '+([a-z])*([a-z0-9])*(-+([a-z0-9]))';

const NEXTJS_DYNAMIC_ROUTES_WITH_MIXED_CASE = `@(${KEBAB_CASE}|\\[${CAMEL_CASE}\\]|\\[\\.\\.\\.${CAMEL_CASE}\\]|\\[\\[\\.\\.\\.${CAMEL_CASE}\\]\\])`;

The idea is that it matches things like

/src/app/help-pages/[helpId]

In other words - things inside of square brackets are camel case while standard routes are kebab case.

Let me know if I can offer any assistance. Again, thank you so much for all the help so far!

@dukeluo dukeluo force-pushed the main branch 2 times, most recently from 036ca7b to 0b88636 Compare June 10, 2023 11:25
@dukeluo
Copy link
Owner

dukeluo commented Jun 12, 2023

@milos-sikic-nimbus-tech Thank you for taking the time to provide such a detailed reply. I finally had a chance to read through your response, and I must say, I learned a lot about Next.js routes from it. I really appreciate your insights.

As you mentioned, there are two options for implementing this support:

  1. Creating a built-in glob pattern that can handle all the cases, allowing users to simply use it like { 'src/**/': 'NEXTJS_ROUTE_CASE' }.
  2. Including a section in the documentation that provides specific glob patterns for users to copy and set.

Personally, I lean towards the first option, despite its potential complexity for developers. I will make an effort to create the glob pattern to match the test cases you provided. If I make any progress or have any updates, I will definitely keep you informed. Once again, thank you for your valuable input.

@milos-sikic-nimbus-tech
Copy link
Contributor Author

Thank you for spending your time on this, @dukeluo! It's much appreciated.

@dukeluo
Copy link
Owner

dukeluo commented Jun 12, 2023

@milos-sikic-nimbus-tech, I wanted to let you know that I have added a commit (d314a19) to incorporate the NEXTJS_ROUTE_CASE support for the "folder-naming-convention" rule. It successfully passes all the cases you provided, except for the "/src/app/somePage.ts" scenario. However, I believe this can be achieved with the assistance of the "filename-naming-convention" rule. Please take a look at the code and feel free to review and make any improvements.

Additionally, I have a couple of questions regarding Next.js routes:

  • Are numbers allowed in Next.js project routes? For example, can we have a path like "src/app/users10/[userId]/page.ts"?
  • Is "NEXTJS_ROUTE_CASE" a suitable name for this type of convention? Is there a more appropriate name?

@milos-sikic-nimbus-tech
Copy link
Contributor Author

milos-sikic-nimbus-tech commented Jun 13, 2023

Hey @dukeluo,

  1. Yeah /src/app/somePage.ts will be picked up by the filename pattern. :)
  2. Yes, numbers are allowed. I just tried /src/app/2-example/page.ts and /src/app/example2/page.ts and they both worked. Basically, whatever can realistically go into a URL can go here, but I think kebab case is the most natural style for URLs (I think numbers should be supported, though).
  3. I think NEXT_JS_APP_ROUTER_CASE would be more fitting, as Next also supports the Page router, which has a different set of rules than the ones than the App router.

@dukeluo
Copy link
Owner

dukeluo commented Jun 14, 2023

Hey @dukeluo,

1. Yeah `/src/app/somePage.ts` will be picked up by the filename pattern. :)

2. Yes, numbers are allowed. I just tried `/src/app/2-example/page.ts` and `/src/app/example2/page.ts` and they both worked. Basically, whatever can realistically go into a URL can go here, but I think kebab case is the most natural style for URLs (I think numbers should be supported, though).

3. I think `NEXT_JS_APP_ROUTER_CASE` would be more fitting, as Next also supports the Page router, which has a different set of rules than the ones than the App router.

I got these, I will update it :)

@dukeluo
Copy link
Owner

dukeluo commented Jun 15, 2023

@milos-sikic-nimbus-tech I have updated my commit, and update the document of the rule folder-naming-convention. I would greatly appreciate it if you could assist me in adding some Next.js context and examples to the documentation. Please feel free to rebase the main branch and make any improvements. Thank you in advance!

@milos-sikic-nimbus-tech
Copy link
Contributor Author

Hey @dukeluo, sorry for the late reply! Will do, as soon as I get some time on my hands. I'll try to do it by Thursday.

@dukeluo
Copy link
Owner

dukeluo commented Jun 19, 2023

Hey @dukeluo, sorry for the late reply! Will do, as soon as I get some time on my hands. I'll try to do it by Thursday.

It's okay, no need to rush. Thank you very much for your help.

@milos-sikic-nimbus-tech
Copy link
Contributor Author

milos-sikic-nimbus-tech commented Jul 5, 2023

Hi @dukeluo, I am so sorry for the delay, I made it a priority to address this today so I managed to push a new commit. Please review and let me know if you'd like me to change anything. :)

Do you think we should mention the pattern in the main README as well?

@dukeluo
Copy link
Owner

dukeluo commented Jul 5, 2023

@milos-sikic-nimbus-tech, I want to express my appreciation for incorporating the additional context and examples into the documentation. I have also made some changes to the documentation regarding the NEXT_JS_APP_ROUTER_CASE naming convention. In order to avoid content overlap, would you be able to rebase the main branch and merge our respective changes? Please feel free to make any necessary improvements during this process.

Since the new builtin pattern NEXT_JS_APP_ROUTER_CASE is specifically designed for the rule folder-naming-convention, it may not be necessary to mention it in the main README.

@milos-sikic-nimbus-tech
Copy link
Contributor Author

@dukeluo Of course, I'll rebase right away!

@milos-sikic-nimbus-tech
Copy link
Contributor Author

Hey @dukeluo, please check again and let me know what you think. :)

@dukeluo
Copy link
Owner

dukeluo commented Jul 5, 2023

@milos-sikic-nimbus-tech Cool. Thanks for you help!

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #27 (0d098bb) into main (bc4f6ec) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main       #27   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          229       229           
=========================================
  Hits           229       229           

@milos-sikic-nimbus-tech
Copy link
Contributor Author

Thank you, @dukeluo! You did most of the work. :)

Again, sorry for the delay and thanks for all the invaluable help!

@milos-sikic-nimbus-tech
Copy link
Contributor Author

@dukeluo Wait, I just noticed an extra newline. I'll fix it ASAP

@milos-sikic-nimbus-tech
Copy link
Contributor Author

@dukeluo Pushed, it's good to go now, unless you want something changed.

@dukeluo dukeluo merged commit bc1d546 into dukeluo:main Jul 5, 2023
@Fabrice-HUET
Copy link

Fabrice-HUET commented Aug 8, 2024

you can use this configuration :

{
"extends": "next/core-web-vitals",
"plugins": [
"folders",
"filenames"
],
"rules": {
"filenames/match-regex": [2, "^(_)?[a-z-]+$"], // kebap-case with optional _ prefix for next.js main files like _app.js
"folders/match-regex": [2, "^(\u005B)?[a-z-]+(\u005D)?$", "/front_end/"], // kebap-case with optional [ ] brackets
}
}

Easier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants