Skip to content

refactor: use setAllMethodNames method#1446

Closed
peixotoleonardo wants to merge 1 commit intonestjs:masterfrom
peixotoleonardo:master
Closed

refactor: use setAllMethodNames method#1446
peixotoleonardo wants to merge 1 commit intonestjs:masterfrom
peixotoleonardo:master

Conversation

@peixotoleonardo
Copy link
Copy Markdown

@peixotoleonardo peixotoleonardo commented Oct 10, 2023

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?

ScheduleExplorer is using scanFromPrototype of the MetadataScanner, this method is deprecated.

Issue Number: N/A

What is the new behavior?

I used the method getAllMethodNames for doing the same thing of the scanFromPrototype method

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@micalevisk
Copy link
Copy Markdown
Member

as that getAllMethodNames method was added in @nestjs/common v9.3.3, I'd say that this is a breaking change for @nestjs/schedule@3 because it would no longer support old versions of @nestjs/common

schedule/package.json

Lines 52 to 54 in d65d1a0

"peerDependencies": {
"@nestjs/common": "^8.0.0 || ^9.0.0 || ^10.0.0",
"@nestjs/core": "^8.0.0 || ^9.0.0 || ^10.0.0",

@sheerlox
Copy link
Copy Markdown
Contributor

as that getAllMethodNames method was added in @nestjs/common v9.3.3, I'd say that this is a breaking change for @nestjs/schedule@3 because it would no longer support old versions of @nestjs/common

schedule/package.json

Lines 52 to 54 in d65d1a0

"peerDependencies": {
"@nestjs/common": "^8.0.0 || ^9.0.0 || ^10.0.0",
"@nestjs/core": "^8.0.0 || ^9.0.0 || ^10.0.0",

stepping in to mention that if there's a breaking change to be released, it might be interesting to bundle it with #1432 and the PR I need to make accordingly. I'll be following the topic.

@peixotoleonardo
Copy link
Copy Markdown
Author

peixotoleonardo commented Oct 13, 2023

as that getAllMethodNames method was added in @nestjs/common v9.3.3, I'd say that this is a breaking change for @nestjs/schedule@3 because it would no longer support old versions of @nestjs/common

schedule/package.json

Lines 52 to 54 in d65d1a0

"peerDependencies": {
"@nestjs/common": "^8.0.0 || ^9.0.0 || ^10.0.0",
"@nestjs/core": "^8.0.0 || ^9.0.0 || ^10.0.0",

I could check if the method getAllMethodNames exists; if it does not exist, I will use the old method.

Or I could check the version of the package @nestjs/common.

@micalevisk
Copy link
Copy Markdown
Member

I could check if the method getAllMethodNames exists; if it does not exist, I will use the old method.

sounds good. Also, add a // TODO(v4): remove this after dropping support for nestjs v9.3.2 somewhere

@peixotoleonardo
Copy link
Copy Markdown
Author

peixotoleonardo commented Oct 13, 2023

I will close this PR because I didn't create a branch to make the changes

#1448

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.

3 participants