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(nuxt): add support for routeRules defined within pages #20391

Merged
merged 20 commits into from Aug 23, 2023

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Apr 19, 2023

πŸ”— Linked issue

❓ 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)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This adds the ability to define routeRules inline, within the pages directory.

Example usage:

<script setup lang="ts">
defineRouteRules({
  ssr: false
  prerender: true,
  headers: {
    'X-Test': 'defined within a page'
  },
})
</script>

TODO:

  • feedback on API
  • implement HMR support in dev mode (currently nitro's runtimeConfig is frozen so we can't update it there). I might consider overriding event.context._nitro.routeRules but this would also miss out getRouteRulesForPath being called directly πŸ€”
  • documentation updated

πŸ“ Checklist

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

@codesandbox
Copy link

codesandbox bot commented Apr 19, 2023

CodeSandbox logoCodeSandbox logoΒ  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@Atinux
Copy link
Member

Atinux commented Apr 19, 2023

I love it!

@danielroe danielroe added this to the v3.5 milestone May 2, 2023
@danielroe danielroe requested review from antfu, Atinux and pi0 May 6, 2023 14:42
@danielroe danielroe marked this pull request as ready for review May 6, 2023 14:43
@danielroe danielroe removed this from the v3.5 milestone May 16, 2023
@Hebilicious
Copy link
Member

I was just thinking about this, would it be possible that we generate some kind of artifact file that reflects the route rules state ?
Something like a routeRules.json, maybe in the app or .nuxt directory. I think it could be helpful to know what's going on as the configuration can come from many different places with this change (ie defineRouteRules can be used through modules/layers). It will be useful to debug what happens, and know how the rules are generated.

Perhaps also add a tab to the devtools ?

@danielroe danielroe added this to the 3.7 milestone Jun 23, 2023
@danielroe
Copy link
Member Author

/ecosystem-ci run sanity-module

@nuxt-ecosystem-ci
Copy link

nuxt-ecosystem-ci bot commented Jul 20, 2023

πŸ“ Ran ecosystem CI: Open

suite result
sanity-module βœ… success

@danielroe
Copy link
Member Author

@pi0 Would love your thoughts here. In dev mode, the first request to any page with inline route rules (that is, the one that triggers a nitro reload) causes a 404 response.

This is also the same issue that #22494 works around - when nitro reloads its dev server it seems to return a 404 to any simultaneous request, rather than 'waiting' for the reload and then returning the response.

tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
test/bundle.test.ts Outdated Show resolved Hide resolved
// Allow telling Nitro to reload route rules
let updateRouteConfig: () => void | Promise<void>
nuxt.hook('nitro:init', (nitro) => {
updateRouteConfig = () => nitro.updateConfig({ routeRules: defu(inlineRules, baseRules) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern would be if other modules are also updating the route rules at runtime, it seems like they would get overridden?

I think the scope for this happening, especially just in development, is pretty low.

Maybe worth merging in nitro.options.routeRules though?

Copy link
Member Author

@danielroe danielroe Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use nitro.options._config.routeRules to access the un-normalised rules.

cc: @pi0 - thoughts on how to avoid clobbering pre-existing route rules?

Copy link
Member

@pi0 pi0 Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess merging with nuxt.options.routeRules would be safer as it contains the latest user (nuxt module) modified rules. In context of Nuxt with top level routeRules, it should be main source of trust.

(we sync runtimeConfig from nuxt to nitro. might need to do a similar fix for route rules as well to make this possible)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitro:config hook is last place for modules to modify nitro config, so that might be safer. (Hence current and previous implementations.)

Otherwise when we update the rules we overwrite intervening changes.

Or maybe use proxy for runtimeConfig/routeRules in resolved options that automatically reloads both within nitro?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modules might need to also apply route rules afterward as well (similar to runtimeConfig). We can either sync references so that nitro and nuxt modules update same object or proxy is also good idea for auto reload πŸ‘πŸΌ (a possible downside is that sometimes we need to batch changes, advising to prefer update config hook is safer)

Copy link
Member Author

@danielroe danielroe Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about instead letting nitro keep track of current state and only merge in/reload the updated rules? That way we wouldn't overwrite anything or need to implement proxy/etc within Nuxt...

@harlan-zw
Copy link
Contributor

Nice πŸ‘

@pi0
Copy link
Member

pi0 commented Aug 22, 2023

This is a really nice initiative! I would recommend to enable it behind an experimental flag until next minor release if want to move forward faster. Coupling with nuxi to support HMR on nuxt.config and also route rule / event handler meta inlined support from Nitro, might affect this implementation and possibly introduce regressions affecting user experience.

@Atinux
Copy link
Member

Atinux commented Aug 23, 2023

It is looking great 😍

Agree to test with an experimental flag until Nuxt 3.8

@danielroe danielroe changed the title feat(nuxt): add support for routeRules defined within pages feat(nuxt): add support for routeRules defined within pages Aug 23, 2023
@danielroe danielroe merged commit 29f4eef into main Aug 23, 2023
25 checks passed
@danielroe danielroe deleted the feat/inline-route-rules branch August 23, 2023 20:38
@github-actions github-actions bot mentioned this pull request Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants