feat(nuxt): sort numbered middleware/plugins from all layers #25906

markthree · 2024-02-22T08:07:33Z

🔗 Linked issue

#25891

❓ Type of change

📖 Documentation (updates to the documentation, readme or JSdoc annotations)
🐞 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

Only sort the global middleware, if there are numbers, they will be moved forward in order, Resolves #25891

📝 Checklist

I have linked an issue or discussion.
I have added tests (if possible).
I have updated the documentation accordingly.

stackblitz · 2024-02-22T08:07:37Z

Run & review this pull request in StackBlitz Codeflow.

packages/nuxt/src/core/templates.ts

Co-authored-by: Damian Głowala <damian.glowala.rebkow@gmail.com>

danielroe

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", 
        
                 ] 
        
               `) 
        
             })

markthree · 2024-02-22T10:33:41Z

@danielroe Thanks. I'll try it.

markthree · 2024-02-22T13:32:44Z

@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) 🤣

packages/nuxt/src/core/app.ts

Co-authored-by: Daniel Roe <daniel@roe.dev>

danielroe · 2024-03-15T15:55:10Z

I'm rethinking this. Plugins explicitly don't behave this way:

nuxt/packages/nuxt/test/app.test.ts

Lines 138 to 148 in 460daa0

    
               expect(fixturePlugins).toMatchInlineSnapshot(` 
        
                 [ 
        
                   "<rootDir>/layer1/plugins/02.plugin.ts", 
        
                   "<rootDir>/layer1/plugins/object-named.ts", 
        
                   "<rootDir>/layer1/plugins/override-test.ts", 
        
                   "<rootDir>/layer2/plugins/01.plugin.ts", 
        
                   "<rootDir>/layer2/plugins/object-named.ts", 
        
                   "<rootDir>/layer2/plugins/override-test.ts", 
        
                   "<rootDir>/plugins/00.plugin.ts", 
        
                   "<rootDir>/plugins/object-named.ts", 
        
                 ]

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.)

markthree · 2024-03-16T01:28:42Z

Thank you @danielroe , I've been so busy lately, I meant to take a look at it when I had some free time. 😭

fix(nuxt): sort global middleware on layers, close nuxt#25891

d173c61

github-actions bot added 3.x bug labels Feb 22, 2024

DamianGlowala reviewed Feb 22, 2024

View reviewed changes

packages/nuxt/src/core/templates.ts Outdated Show resolved Hide resolved

Update packages/nuxt/src/core/templates.ts

71ad1f8

Co-authored-by: Damian Głowala <damian.glowala.rebkow@gmail.com>

DamianGlowala changed the title ~~fix(nuxt): sort global middleware on layers, close #25891~~ fix(nuxt): sort global middleware in layers Feb 22, 2024

danielroe requested changes Feb 22, 2024

View reviewed changes

markthree added 2 commits February 22, 2024 21:22

refactor(nuxt): resolves layer middleware in correct order

4a96c6c

chore(nuxt): fmt

ae4df04

markthree changed the title ~~fix(nuxt): sort global middleware in layers~~ fix(nuxt): resolves layer middleware in correct order Feb 22, 2024

DamianGlowala changed the title ~~fix(nuxt): resolves layer middleware in correct order~~ fix(nuxt): resolve layer middleware in correct order Feb 22, 2024

fix(nuxt): fix typo

7a219a7

danielroe reviewed Feb 22, 2024

View reviewed changes

packages/nuxt/src/core/app.ts Outdated Show resolved Hide resolved

markthree and others added 5 commits February 22, 2024 22:37

Update packages/nuxt/src/core/app.ts

81c31c0

Co-authored-by: Daniel Roe <daniel@roe.dev>

Merge remote-tracking branch 'origin/main' into markthree/main

1823a51

style: lint

3a708e5

fix: require dot after number to mark middleware as ordered;

9576fed

perf: hoist out function

85e9ba6

danielroe approved these changes Mar 15, 2024

View reviewed changes

danielroe changed the title ~~fix(nuxt): resolve layer middleware in correct order~~ fix(nuxt): sort numbered middleware by number Mar 15, 2024

danielroe changed the title ~~fix(nuxt): sort numbered middleware by number~~ fix(nuxt): sort numbered middleware Mar 15, 2024

[autofix.ci] apply automated fixes

460daa0

danielroe marked this pull request as draft March 15, 2024 16:33

danielroe added 3 commits March 15, 2024 13:55

Merge remote-tracking branch 'origin/main' into markthree/main

fe76e9b

fix: test path rather than middleware name

e60d6bd

fix: sort plugins and improve perf

76b129c

danielroe marked this pull request as ready for review March 15, 2024 21:21

danielroe approved these changes Mar 15, 2024

View reviewed changes

danielroe changed the title ~~fix(nuxt): sort numbered middleware~~ feat(nuxt): sort numbered middleware/plugins from all layers Mar 15, 2024

eunjae-lee mentioned this pull request Mar 28, 2024

feat(common): add afterAuthenticated middleware hook storyblok/space-tool-plugins#40

Merged

markthree added 2 commits April 6, 2024 19:26

fix: support overriding named plugins

9907f33

fix: the layer's plugin takes precedence

0bd2f31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nuxt): sort numbered middleware/plugins from all layers #25906

feat(nuxt): sort numbered middleware/plugins from all layers #25906

markthree commented Feb 22, 2024 •

edited

stackblitz bot commented Feb 22, 2024

danielroe left a comment

markthree commented Feb 22, 2024

markthree commented Feb 22, 2024

danielroe commented Mar 15, 2024 •

edited

markthree commented Mar 16, 2024 •

edited

	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') })
	}
	}

	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('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",
	]
	`)
	})

feat(nuxt): sort numbered middleware/plugins from all layers #25906

Are you sure you want to change the base?

feat(nuxt): sort numbered middleware/plugins from all layers #25906

Conversation

markthree commented Feb 22, 2024 • edited

🔗 Linked issue

❓ Type of change

📚 Description

📝 Checklist

stackblitz bot commented Feb 22, 2024

danielroe left a comment

Choose a reason for hiding this comment

markthree commented Feb 22, 2024

markthree commented Feb 22, 2024

danielroe commented Mar 15, 2024 • edited

markthree commented Mar 16, 2024 • edited

markthree commented Feb 22, 2024 •

edited

danielroe commented Mar 15, 2024 •

edited

markthree commented Mar 16, 2024 •

edited