Skip to content

fix: add opt-in appendAutoPrefix for directory auto prefixes - #498

Merged
jean-michelet merged 6 commits into
fastify:mainfrom
SergioChan:fix-issue-205-append-auto-prefix
Jul 24, 2026
Merged

jean-michelet merged 6 commits into
fastify:mainfrom
SergioChan:fix-issue-205-append-auto-prefix

Conversation

@SergioChan

@SergioChan SergioChan commented Mar 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • add a new appendAutoPrefix autoload option (default false)
  • keep existing behavior by default (plugin.autoPrefix still overrides directory-derived prefixes)
  • when appendAutoPrefix: true, concatenate directory prefix + plugin.autoPrefix
  • add Issue autoPrefix overwrites directory path set by dirNameRoutePrefix #205 coverage to verify both default and opt-in behaviors
  • document the new option and update TypeScript definitions

Issue

Closes #205 and close this PR #350

Validation

  • node --test test/issues/205/test.js
  • node --test test/commonjs/basic.js

Comment thread README.md Outdated
Comment thread README.md
@SergioChan

Copy link
Copy Markdown
Contributor Author

Thanks for the review — I pushed an update to clarify the behavior and example in README.

What changed:

  • Rewrote the appendAutoPrefix section to explicitly describe both modes (false vs true).
  • Added a concrete directory example showing routes/children/new-routes.js exporting autoPrefix = '/batch'.
  • Added explicit resulting route paths for both modes:
    • default: /your-parent-prefix/batch/entity
    • append mode: /your-parent-prefix/children/batch/entity

Validation:

  • node --test test/issues/205/test.js

If you want, I can also add a short one-line cross-reference from dirNameRoutePrefix to appendAutoPrefix for discoverability.

@Tony133
Tony133 requested a review from mcollina July 16, 2026 10:15

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Comment thread test/issues/205/test.js Outdated
Comment thread README.md
@Tony133

Tony133 commented Jul 16, 2026

Copy link
Copy Markdown
Member

@SergioChan as soon as you have time, could you resolve the two issues that were flagged? The one regarding the tests is the priority, at the very least.

Signed-off-by: Yuheng <cyh9211@icloud.com>
@SergioChan

Copy link
Copy Markdown
Contributor Author

@Tony133 resolved both flagged threads.

  • Tests: flattened the nested describe blocks into two direct it cases in 4dba83c, with each test managing its own Fastify lifecycle.
  • API suggestion: replied with the rationale for keeping appendAutoPrefix as the global opt-in proposed in issue autoPrefix overwrites directory path set by dirNameRoutePrefix #205; the suggestion was non-blocking and the current behavior remains explicitly documented.

Validation:

  • npm run lint
  • node --test test/issues/205/test.js
  • npm test (277 unit tests passed, 100% coverage)

Both review threads are now resolved.

@Tony133

Tony133 commented Jul 16, 2026

Copy link
Copy Markdown
Member

@Tony133 resolved both flagged threads.

  • Tests: flattened the nested describe blocks into two direct it cases in 4dba83c, with each test managing its own Fastify lifecycle.
  • API suggestion: replied with the rationale for keeping appendAutoPrefix as the global opt-in proposed in issue autoPrefix overwrites directory path set by dirNameRoutePrefix #205; the suggestion was non-blocking and the current behavior remains explicitly documented.

Validation:

  • npm run lint
  • node --test test/issues/205/test.js
  • npm test (277 unit tests passed, 100% coverage)

Both review threads are now resolved.

Thanks!

@Tony133

Tony133 commented Jul 16, 2026

Copy link
Copy Markdown
Member

@SergioChan one last change: can you update the test so we remove the try/finally block in favor of t.after()? It keeps the test even flatter and cleaner:

'use strict'

const { describe, it } = require('node:test')
const assert = require('node:assert')
const path = require('node:path')
const Fastify = require('fastify')
const autoLoad = require('../../../')

describe('Issue 205: append autoPrefix to directory prefixes without breaking defaults', function () {
  it('should keep autoPrefix overriding directory prefixes by default', async function (t) {
    const app = Fastify()
    t.after(() => app.close())

    app.register(autoLoad, {
      dir: path.join(__dirname, 'routes'),
      options: { prefix: '/hooked-plugin' }
    })
    await app.ready()

    const overridden = await app.inject({ method: 'GET', url: '/hooked-plugin/batch/entity' })
    assert.strictEqual(overridden.statusCode, 200)
    assert.deepStrictEqual(overridden.json(), { ok: true })

    const appended = await app.inject({ method: 'GET', url: '/hooked-plugin/children/batch/entity' })
    assert.strictEqual(appended.statusCode, 404)
  })

  it('should concatenate directory prefixes before plugin autoPrefix when appendAutoPrefix is true', async function (t) {
    const app = Fastify()
    t.after(() => app.close())

    app.register(autoLoad, {
      dir: path.join(__dirname, 'routes'),
      options: { prefix: '/hooked-plugin' },
      appendAutoPrefix: true
    })
    await app.ready()

    const appended = await app.inject({ method: 'GET', url: '/hooked-plugin/children/batch/entity' })
    assert.strictEqual(appended.statusCode, 200)
    assert.deepStrictEqual(appended.json(), { ok: true })

    const overridden = await app.inject({ method: 'GET', url: '/hooked-plugin/batch/entity' })
    assert.strictEqual(overridden.statusCode, 404)
  })
})

Signed-off-by: Yuheng <cyh9211@icloud.com>
@SergioChan

Copy link
Copy Markdown
Contributor Author

@Tony133 addressed in 0653c09. Both tests now register Fastify cleanup with t.after(() => app.close()), and the try/finally blocks have been removed as suggested.

Validation:

  • npm run lint
  • node --test test/issues/205/test.js
  • npm test (277 unit tests passed, 100% coverage)

@jean-michelet
jean-michelet merged commit cafd07e into fastify:main Jul 24, 2026
16 checks passed
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.

autoPrefix overwrites directory path set by dirNameRoutePrefix

4 participants