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(core): add option to have router module path before version #13288

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tbedrnik
Copy link

@tbedrnik tbedrnik commented Mar 4, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

When using RouterModule along with URI versioning, you cannot affect the order of path parts. It is always like this:
/{globalAppPrefix}/{version}/{modulePath}/{controllerPath}/{methodPath}

I'm using RouterModule to separate public facing and internal routes like this:

// app.module.ts
@Module({
  imports: [
    PublicApiModule,
    InternalApiModule,
    RouterModule.register([
      {
        path: 'public',
        module: PublicApiModule,
      },
      {
        path: 'internal',
        module: InternalApiModule,
      },
    ]),
  ],
})
export class AppModule {}

// main.ts
app.setGlobalPrefix('api')
app.enableVersioning({type: VersioningType.URI})

So my routes are /api/public/... and /api/internal/....
When I enable URI versioning my routes are /api/v1/public/... and /api/v1/internal/... which breaks my proxy which enables any requests starting with /api/public/* to pass in.

Issue Number: N/A

What is the new behavior?

After adding pathBeforeVersion: true, the module path and version parts are swapped like so:
/{globalAppPrefix}/{modulePath}/{version}/{controllerPath}/{methodPath}

@Module({
  imports: [
    PublicApiModule,
    InternalApiModule,
    RouterModule.register([
      {
        path: 'public',
        pathBeforeVersion: true,
        module: PublicApiModule,
      },
      {
        path: 'internal',
        pathBeforeVersion: true,
        module: InternalApiModule,
      },
    ]),
  ],
})
export class AppModule {}

My routes are now /api/public/v1/... and /api/internal/v1/... and my proxy still works.

Setting the pathBeforeVersion: false or omitting the parameter doesn't introduce any change.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@tbedrnik tbedrnik force-pushed the feat-router-module-version-position branch from 962d699 to 8a2bd2e Compare March 4, 2024 17:40
@coveralls
Copy link

Pull Request Test Coverage Report for Build c036f711-58ff-4d30-9953-75a598aeb263

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 92.172%

Totals Coverage Status
Change from base Build 2bf2e4c8-5da9-4dd1-8f93-3b733029afc6: 0.01%
Covered Lines: 6747
Relevant Lines: 7320

💛 - Coveralls

@micalevisk
Copy link
Member

micalevisk commented Mar 4, 2024

I'm not sure about the API

I found that pathBeforeVersion flag a bit hacky (like a patch over something) and not flexible.

As versioning is a first-class thing in nestjs, I think we should use this concept somehow here but I didn't have a suggestion in mind so far

@tbedrnik
Copy link
Author

tbedrnik commented Mar 5, 2024

I'm not sure about the API

I found that pathBeforeVersion flag a bit hacky (like a patch over something) and not flexible.

As versioning is a first-class thing in nestjs, I think we should use this concept somehow here but I didn't have a suggestion in mind so far

It could be something like versionPosition: 'AFTER' | 'BEFORE'.

Or another idea would be to extend the versioning options which could give more options for the position of the version within the route, but doesn't offer the granularity of setting this per RouterModule.

export interface UriVersioningOptions {
  type: VersioningType.URI;
  /**
   * Optional prefix that will prepend the version within the URI.
   *
   * Defaults to `v`.
   *
   * Ex. Assuming a version of `1`, for `/api/v1/route`, `v` is the prefix.
   */
  prefix?: string | false;

  // Adding this:
  position?: UriVersionPosition;
}

enum UriVersionPosition {
  AFTER_GLOBAL_PREFIX,
  AFTER_MODULE,
  // Theoretically able to have these as well:
  AFTER_CONTROLLER,
  AFTER_ROUTE,
}

Actually both could be combined and RouterModule's setting would be prioritized.

I'm in to coming up with something more flexible. It's just about how flexible we want this to be. Currently before or after RouterModule's path seems enough to me, but maybe somebody would like it after their Controller's path.

@benjGam
Copy link

benjGam commented Mar 12, 2024

I'm not sure about the API
I found that pathBeforeVersion flag a bit hacky (like a patch over something) and not flexible.
As versioning is a first-class thing in nestjs, I think we should use this concept somehow here but I didn't have a suggestion in mind so far

It could be something like versionPosition: 'AFTER' | 'BEFORE'.

Or another idea would be to extend the versioning options which could give more options for the position of the version within the route, but doesn't offer the granularity of setting this per RouterModule.

export interface UriVersioningOptions {
  type: VersioningType.URI;
  /**
   * Optional prefix that will prepend the version within the URI.
   *
   * Defaults to `v`.
   *
   * Ex. Assuming a version of `1`, for `/api/v1/route`, `v` is the prefix.
   */
  prefix?: string | false;

  // Adding this:
  position?: UriVersionPosition;
}

enum UriVersionPosition {
  AFTER_GLOBAL_PREFIX,
  AFTER_MODULE,
  // Theoretically able to have these as well:
  AFTER_CONTROLLER,
  AFTER_ROUTE,
}

Actually both could be combined and RouterModule's setting would be prioritized.

I'm in to coming up with something more flexible. It's just about how flexible we want this to be. Currently before or after RouterModule's path seems enough to me, but maybe somebody would like it after their Controller's path.

This is an interesting approach, we should consider it instead of the OC solution, i mean, it seems to be permissive (Also seems a little bit useless in 99% but for NestJS as a framework, it could be better to offer this kind of change instead of just one different placement slot)

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.

None yet

4 participants