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

7.0.0 #593

Merged
merged 30 commits into from
Apr 5, 2020
Merged

7.0.0 #593

merged 30 commits into from
Apr 5, 2020

Conversation

BrunnerLivio
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[x] Feature
[x] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[x] Build related changes
[x] CI related changes
[x] Release, baby

What is the current behavior?

Issue Number: #9 #32 #33 #312 #340 #561 #569 #580

What is the new behavior?

One of the main goals I want to tackle is the very complicated JSON object for the TerminusModule.forRootAsync.

v6

@Module({
  imports: [
    TerminusModule.forRootAsync({
      useFactory: (disk: DiskHealthIndicator) => {
        return {
          endpoints: [
            {
              url: '/health',
              healthIndicators: [
                async () =>
                  disk.checkStorage('disk', {
                    path: '/',
                    thresholdPercent: 0.5,
                  }),
              ],
            },
          ],
        };
      },
      inject: [DiskHealthIndicator],
    }),
  ],
})
export class AppModule {}

v7

@Controller('health')
export class HealthController {
  constructor(
    private healthService: HealthService,
    private disk: DiskHealthIndicator,
  ) {}

  @Get()
  @HealthCheck()
  readiness() {
    return this.healthService.check([
      () => this.disk.checkStorage('disk', { path: '/', thresholdPercent: 0.5 }),
    ]);
  }
}

The v6 way of registering health checks will mostly stay in the package without any further action needed for most users. It will get completely removed in v8.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Todo @BrunnerLivio

@tristan957
Copy link

@BrunnerLivio thanks for your work on this. Cannot wait to include it in my application.

@BrunnerLivio
Copy link
Member Author

BrunnerLivio commented Mar 23, 2020

@neverson-silva hopefully sometime this week. You can already opt-in to use the prerelease though

npm i -s @nestjs/terminus@7.0.0-pre.2

See migration guide (work in progress). Though before you migrate, can you check whether everything works fine just by upgrading the dependency? It should be backward-compatible and only print a deprecation warning.

@neverson-silva
Copy link

neverson-silva commented Mar 23, 2020

@BrunnerLivio thanks for your reply, everything worked very well. I'll read the migration guide.

@geekflyer
Copy link

Hey, I noticed an issue with the new v7 syntax.
In our app we've set a global prefix via app.setGlobalPrefix("api/v1");. With the previous syntax the health endpoint was available under /health but with the new syntax that endpoint moved to /api/v1/health which is kinda unintentional. I didn't find a way to prevent the health check from picking up the global prefix. Is there a way to get the old behaviour?

@BrunnerLivio
Copy link
Member Author

BrunnerLivio commented Mar 24, 2020

@geekflyer Great catch, I will need to add this as a note to the migration guide!

Unfortunately, I can’t. Though you can:

  • Use the legacy API until nestjs/nest#963 is resolved (the legacy API will not break in terminus v7, it will be removed in v8)

  • Don’t let NestJS handle global prefix and instead use a simple static variable which you prepend for each controller.

  • Use a rather hacky workaround like this:

// my-health.service.ts
@Injectable()
export class MyHealthService {
  constructor(
    private health: HealthCheckService,
    private dns: DNSHealthIndicator,
  ) {}
  healthcheck() {
    return this.health.check([
      () => this.dns.pingCheck('nestjs', 'https://nestjs.com'),
    ]);
  }
}

// main.ts
async function bootstrap() {
  const app = await NestFactory.create(AppModule);
  app.enableShutdownHooks();

  const myHealthService = app.get(MyHealthService);
  app.getHttpAdapter().get('/health', async (_, res) => {
    try {
      const result = await myHealthService.healthcheck();
      return res.status(200).json(result);
    } catch (error) {
      return res.status(500).json(error.response);
    }
  });

  await app.listen(3000);
}
bootstrap();

@geekflyer
Copy link

ok, let's say i wanna continue using the old syntax, is there a way to disable the deprecation warning?

@BrunnerLivio BrunnerLivio force-pushed the releases/7.0.0 branch 2 times, most recently from 08e0c74 to 0a75913 Compare March 25, 2020 22:15
@BrunnerLivio
Copy link
Member Author

@geekflyer added an environment variable TERMINUS_IGNORE_DEPRECATED=true to disable the deprecation warnings in @nestjs/terminus@7.0.0-pre.3

@geekflyer
Copy link

@BrunnerLivio , thanks, but I wonder could you rather make this a parameter of the forRootAsync method? something like:

 TerminusModule.forRootAsync({
      ignoreDeprecationWarning: true,
      useFactory: (disk: DiskHealthIndicator) => {
        return {
          endpoints: [
            {
              url: '/health',
              healthIndicators: [
                async () =>
                  disk.checkStorage('disk', {
                    path: '/',
                    thresholdPercent: 0.5,
                  }),
              ],
            },
          ],
        };
      },
      inject: [DiskHealthIndicator],
    }),

or

 TerminusModule.forRootAsync({
      useFactory: (disk: DiskHealthIndicator) => {
        return {
          ignoreDeprecationWarning: true,
          endpoints: [
            {
              url: '/health',
              healthIndicators: [
                async () =>
                  disk.checkStorage('disk', {
                    path: '/',
                    thresholdPercent: 0.5,
                  }),
              ],
            },
          ],
        };
      },
      inject: [DiskHealthIndicator],
    }),

I think using an environment variable to suppress the deprecation warning seems a bit heavy handed :)

@BrunnerLivio
Copy link
Member Author

@geekflyer personally I don’t really like that (seems to me a little bit unintuitive design), but I can’t come up with a strong unopinionated counter argument either haha. Since these options will anyway get removed in v8 I don’t really care. Therefore I’ll refactor the env variable asap

@geekflyer
Copy link

awesome thanks!

@BrunnerLivio
Copy link
Member Author

@geekflyer disableDeprecationWarnings has been added to the TerminusOptions.


@ everyone: The PR is almost done. If you are able to check out the pre-release npm install @nestjs/terminus@7.0.0-pre.4, please do so, and give me more feedback!

@BrunnerLivio BrunnerLivio changed the title WIP: 7.0.0 7.0.0 Mar 28, 2020
@BrunnerLivio BrunnerLivio merged commit 19c3483 into master Apr 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the releases/7.0.0 branch April 5, 2020 15:45
@BrunnerLivio
Copy link
Member Author

BrunnerLivio commented Apr 5, 2020

Terminus v7 has been released! Migration guide to be followed asap.
Thanks, everyone for your patience and really helpful support! ❤️

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

6 participants