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

feat: support regex for prerender.ignore #1033

Closed
wants to merge 4 commits into from
Closed

Conversation

thezzisu
Copy link

@thezzisu thezzisu commented Mar 7, 2023

πŸ”— Linked issue

Resolves #308

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Enhance prerender.ignore with the ability to use RegExp.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@thezzisu thezzisu marked this pull request as ready for review March 7, 2023 13:54
@nuxt-studio
Copy link
Contributor

nuxt-studio bot commented Mar 7, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
nitro Edit on Studio β†—οΈŽ View Live Preview 91b4061

@thezzisu
Copy link
Author

thezzisu commented Mar 7, 2023

Notice: maybe conflict with #1032

@atinux
Copy link
Member

atinux commented Mar 8, 2023

Will first merge #1032 to rebase your branch

@atinux
Copy link
Member

atinux commented Mar 8, 2023

Fixed the conflicts and improve the docs at the same time.

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #1033 (91b4061) into main (554b358) will decrease coverage by 0.05%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##             main    #1033      +/-   ##
==========================================
- Coverage   67.59%   67.55%   -0.05%     
==========================================
  Files          62       62              
  Lines        6259     6263       +4     
  Branches      707      707              
==========================================
  Hits         4231     4231              
- Misses       2015     2019       +4     
  Partials       13       13              
Impacted Files Coverage Ξ”
src/prerender.ts 88.13% <0.00%> (-1.22%) ⬇️
src/types/nitro.ts 100.00% <100.00%> (ΓΈ)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@atinux atinux requested review from pi0 and atinux March 8, 2023 10:02
@atinux
Copy link
Member

atinux commented Mar 8, 2023

Wondering if we could add a test case for it as well.

@pi0 pi0 changed the title feat: enhance prerender.ignore feat: support regex for prerender.ignore Mar 8, 2023
@pi0
Copy link
Member

pi0 commented Mar 8, 2023

I'm thinking we might be better first allow ** and * in ignore patterns to keep it consistent with route rules. We can also introduce a hook / function filter to allow ignoring routes on demand for more advanced cases.

Copy link
Member

atinux commented Mar 8, 2023

Allowing the pattern ** and * will solve your usecase @thezzisu ?

@pi0
Copy link
Member

pi0 commented Mar 8, 2023

I'm afraid not. According to linked issue, it requires custom logic which can be implemented by a custom hook / callback (internally can use also regex...)

Copy link
Member

atinux commented Mar 8, 2023

So what are the next steps?

@thezzisu
Copy link
Author

thezzisu commented Mar 8, 2023

As @pi0 said, custom hooks/callbacks would be the most powerful solution; but, for most use cases, RegEx should be enough.

Allowing the pattern ** and * will solve your usecase @thezzisu ?

In fact, my use cases needs the power of "excluded ignore", which is not easily solvable with route rules matching. The underlying library, whose radix-tree-matching do not produce result with the same order as in the input.

@thezzisu
Copy link
Author

thezzisu commented Mar 8, 2023

Wondering if we could add a test case for it as well.

Should I implement more strict input checking? If the user uses TypeScript, the current solution will work fine.

Copy link
Member

atinux commented Mar 9, 2023

I think it's fine for types using TS.

If I understand correctly, you don't want to support regex but the ability to give a function instead @pi0 ?

@pi0
Copy link
Member

pi0 commented Mar 10, 2023

A hook would be nice for advanced usecases.

I prefer to keep ignore strict string pattern format as much as possible. This (same as route rules) is an advantage to be able static rules to provider rules which by introducing regexes to route defs we lose.

What do you think about introducing prerender.include as well:

prerender: {
  include: ['/static/**'],
  exclude: ['/**']
}

@thezzisu
Copy link
Author

A hook would be nice for advanced usecases.

I prefer to keep ignore strict string pattern format as much as possible. This (same as route rules) is an advantage to be able static rules to provider rules which by introducing regexes to route defs we lose.

What do you think about introducing prerender.include as well:

prerender: {
  include: ['/static/**'],
  exclude: ['/**']
}

But how about the priority? Is the 'include' a 'negative exclude' like !blahblah syntax in .gitignore?

Since this option won't affect how route options work (in fact, it's a option for the crawler, not the server), I think RegExs should be fine.

@pi0
Copy link
Member

pi0 commented Mar 13, 2023

But how about the priority?

The most precise rule takes into priority

Since this option won't affect how route options work

The point is route rule definition consistency and having most simple way to do things when possible.

@thezzisu
Copy link
Author

But how about the priority?

The most precise rule takes into priority

Since this option won't affect how route options work

The point is route rule definition consistency and having most simple way to do things when possible.

OK. How about add a prerenderRules like routeRules that using the similar syntax Record<string, SomeOptions>?

@thezzisu
Copy link
Author

But how about the priority?

The most precise rule takes into priority

Since this option won't affect how route options work

The point is route rule definition consistency and having most simple way to do things when possible.

OK. How about add a prerenderRules like routeRules that using the similar syntax Record<string, SomeOptions>?

The reason is that, routeRules is for server, but the prerender is a standalone thing though.

@pi0
Copy link
Member

pi0 commented Mar 13, 2023

We do actually support prerender: true for routeRules (route rules are unified routing rules not limited to server).

We can indeed support prerender: false to allow overiridng for specific patterns πŸ‘πŸΌ

@thezzisu
Copy link
Author

We do actually support prerender: true for routeRules (route rules are unified routing rules not limited to server).

We can indeed support prerender: false to allow overiridng for specific patterns πŸ‘πŸΌ

So the problem is I want to make / not prerendered (SSR) when using the server built by nuxt build, but to be prerendered when using nuxt generate.

@pi0
Copy link
Member

pi0 commented Mar 14, 2023

Yes that would be possible! (SSR renderer won't be affected by prerender rule only nitro prerender/nuxt generate uses this specific rule).

@thezzisu
Copy link
Author

Sorry for the delay. Now the problems becomes that the https://github.com/unjs/radix3#route-matcher is not matching the most specific rule! Let's see a example below:

image

image

Simple swapping the rules will affect the output.

Copy link
Member

atinux commented Mar 21, 2023

Nice catch @thezzisu

It seems we need a way to prioritize the routes for best matching strategy, similar to what vue-router 4 is doing πŸ€”

@thezzisu
Copy link
Author

I think this would be a better solution than supporting regex. I'll close this pull request, and open a issue on unjs/radix3 instead.

@pi0
Copy link
Member

pi0 commented Mar 22, 2023

Radix 3 basically does that :) but we don't use it internally in nitro prerender inference...

Copy link
Member

atinux commented Mar 22, 2023

So open back an issue in Nitro pre-render to support this I guess πŸ˜…

@pi0
Copy link
Member

pi0 commented Mar 22, 2023

~> #1078

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.

allow excluding routes from prerenderer crawling
3 participants