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

fix: throw an error when there are no controllers found #1303

Merged
merged 9 commits into from
Aug 30, 2022

Conversation

simllll
Copy link
Contributor

@simllll simllll commented Aug 29, 2022

With the ts4.8 issue it turns out tsoa does not throw any kind of error if there are zero controllers found. Which makes it even a bit hard to find the issue in first place. To prevent this in the future I added a check that throws an Error if there are no controllers discovered.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
  • This PR is associated with an existing issue?

Closing issues

Put closes #XXXX (where XXXX is the issue number) in your comment to auto-close the issue that your PR fixes.

potentially helps e.g. for #1301

If this is a new feature submission:

  • Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach

In my opinion it doesn't make sense to use tsoa without any controllers, but maybe I'm missing some special use cases?

Test plan

the added test catches the expected error when there are no controllers

@simllll simllll closed this Aug 29, 2022
@simllll simllll reopened this Aug 29, 2022
@WoH
Copy link
Collaborator

WoH commented Aug 29, 2022

TypeStrong/ts-node#1839

@simllll simllll changed the title fix: throw error when there are no controllers found fix: throws error when there are no controllers found Aug 29, 2022
@simllll simllll changed the title fix: throws error when there are no controllers found fix: throw an error when there are no controllers found Aug 29, 2022
@simllll
Copy link
Contributor Author

simllll commented Aug 29, 2022

quite some dependency updates needed to get this running again.

  • dropped node 12 from test matrix (as it does not work with node 12 anymore)
  • upgraded all deps (except chalk because it's esm only in version 5) to latest

should we maybe move that to a seperat PR?

as a side effect this will also temporary fix any issues with ts4.8 (as 4.7 will be used ;-))
@WoH
Copy link
Collaborator

WoH commented Aug 29, 2022

@simllll
Copy link
Contributor Author

simllll commented Aug 30, 2022

It seems though it's only related to windows, it finishes successfully on linux and mac on the github actions (locally also on linux, tests are running fine).

I guess it has something todo with the paths, as on windows the allFiles array is empty:

importClassesFromDirectories [ 'fixtures/inversify-cpg/*Controller.ts' ] [ '.ts' ]
allFiles []

issue found and fixed:
image

@simllll simllll force-pushed the test/no-controllers branch 2 times, most recently from 7719215 to 77cf57a Compare August 30, 2022 07:35
@simllll
Copy link
Contributor Author

simllll commented Aug 30, 2022

Still fails on windows somehow with superagent.

tsoa-tests: err Error: read ECONNRESET
tsoa-tests:     at TCP.onStreamRead (node:internal/stream_base_commons:217:20) {
tsoa-tests:   errno: -4077,
tsoa-tests:   code: 'ECONNRESET',
tsoa-tests:   syscall: 'read',
tsoa-tests:   response: undefined
tsoa-tests: }

first was thinking this could be related ladjs/supertest#617 but there is actually no http2.. kinda hard to figure it out on a non-windows env...
maybe related ladjs/supertest#709

@WoH WoH merged commit 59a2cc3 into lukeautry:master Aug 30, 2022
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.

2 participants