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): sort numbered middleware/plugins from all layers #25906
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
Co-authored-by: Damian GΕowala <damian.glowala.rebkow@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! β€οΈ
We create the middleware here:
nuxt/packages/nuxt/src/core/app.ts
Lines 130 to 143 in b96fe1e
app.middleware = [] | |
for (const config of reversedConfigs) { | |
const middlewareDir = (config.rootDir === nuxt.options.rootDir ? nuxt.options : config).dir?.middleware || 'middleware' | |
const middlewareFiles = await resolveFiles(config.srcDir, `${middlewareDir}/*{${nuxt.options.extensions.join(',')}}`) | |
for (const file of middlewareFiles) { | |
const name = getNameFromPath(file) | |
if (!name) { | |
// Ignore files like `~/middleware/index.vue` which end up not having a name at all | |
logger.warn(`No middleware name could be resolved for \`~/${relative(nuxt.options.srcDir, file)}\`. Bear in mind that \`index\` is ignored for the purpose of creating a middleware name.`) | |
continue | |
} | |
app.middleware.push({ name, path: file, global: hasSuffix(file, '.global') }) | |
} | |
} |
We could sort them there, but I think it would (also?) be better to sort the results coming from resolveFiles
:
nuxt/packages/kit/src/resolve.ts
Lines 163 to 166 in a2ef309
export async function resolveFiles (path: string, pattern: string | string[], opts: { followSymbolicLinks?: boolean } = {}) { | |
const files = await globby(pattern, { cwd: path, followSymbolicLinks: opts.followSymbolicLinks ?? true }) | |
return files.map(p => resolve(path, p)).filter(p => !isIgnored(p)).sort() | |
} |
It would also be amazing if you could add a test case here:
nuxt/packages/nuxt/test/app.test.ts
Lines 152 to 183 in b784336
it('resolves layer middleware in correct order', async () => { | |
const app = await getResolvedApp([ | |
// layer 1 | |
'layer1/middleware/global.global.ts', | |
'layer1/middleware/named-from-layer.ts', | |
'layer1/middleware/named-override.ts', | |
'layer1/nuxt.config.ts', | |
// layer 2 | |
'layer2/middleware/global.global.ts', | |
'layer2/middleware/named-from-layer.ts', | |
'layer2/middleware/named-override.ts', | |
'layer2/plugins/override-test.ts', | |
'layer2/nuxt.config.ts', | |
// final (user) layer | |
'middleware/named-override.ts', | |
'middleware/named.ts', | |
{ | |
name: 'nuxt.config.ts', | |
contents: 'export default defineNuxtConfig({ extends: [\'./layer2\', \'./layer1\'] })' | |
} | |
]) | |
const fixtureMiddleware = app.middleware.filter(p => p.path.includes('<rootDir>')).map(p => p.path) | |
// TODO: fix this | |
expect(fixtureMiddleware).toMatchInlineSnapshot(` | |
[ | |
"<rootDir>/layer2/middleware/global.global.ts", | |
"<rootDir>/layer2/middleware/named-from-layer.ts", | |
"<rootDir>/middleware/named-override.ts", | |
"<rootDir>/middleware/named.ts", | |
] | |
`) | |
}) |
@danielroe Thanks. I'll try it. |
@danielroeοΌIt's done, and while this way of writing it isn't the best for performance, it's clear enough and also introduces as little break change as possible. π₯° Maybe in the future we can have resolveFiles upper level tools that can handle it, but I can't think of a good design right now (without introducing break change) π€£ |
I'm rethinking this. Plugins explicitly don't behave this way: nuxt/packages/nuxt/test/app.test.ts Lines 138 to 148 in 460daa0
I think if we want numbered plugins/middleware to be sorted and hoisted out, they should have the same behaviour. Would you be able to update this PR so both are handled the same? (Check #22889 for context.) |
Thank you @danielroe , I've been so busy lately, I meant to take a look at it when I had some free time. π |
π Linked issue
#25891
β Type of change
π Description
Only sort the global middleware, if there are numbers, they will be moved forward in order, Resolves #25891
π Checklist