feat(common): more verbose tsc error message on wrong provider declaration

[micalevisk]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

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?

having the follow custom provider:

{
  provide: '',
  useClass: class {},
  inject: [],
}

triggers the TSC error:

which isn't that friendly for beginners on TS

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

I know that this kind of usage looks hacky and isn't common but I believe it is less cryptic (or maybe is not) :p

Inspired by the video CUSTOM ERRORS in TypeScript? - Advanced TypeScript

[coveralls]

Pull Request Test Coverage Report for Build 4cc0497a-cc99-46e5-a464-7a7b852d20df

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 93.826%

---

Totals
Change from base Build 7429007a-3efb-492d-a2a1-84f706a41b5f:	0.0%
Covered Lines:	6094
Relevant Lines:	6495

---

💛 - Coveralls

[kamilmysliwiec]

Interesting - I'm wondering if that's a common practice to use this trick. What do you think about this @jmcdo29?

[jmcdo29]

It doesn't seem common, but it does seem useful. I like the approach because of the more verbose error message too, and as we're already using never it shouldn't be a breaking change to swap over to this custom string

[kamilmysliwiec]

I wonder if it's clean from TS (language) perspective. String Literal Types were not designed to serve as better error messages. Technically if we approve this PR, the following code will be OK from the compiler's/type definition standpoint:

{
  provide: {},
  useClass: RandomClass,
  inject: 'The `inject` option is only for factory providers!',
}

I'm not saying that I dislike it as it clearly makes the error message verbose, just thinking out loud.

[jmcdo29]

That shouldn't be possible, right? We're using the string literal as a type on ClassProvider and ValueProvider, it shouldn't bleed into the FactoryProvider type, right?

[kamilmysliwiec]

Just updated the code snippet @jmcdo29, I was referring to class-based providers of course! Thanks for pointing that out

[jmcdo29]

Ah, okay. So yeah, technically someone could do that. What would be the implications if they did? We actually ignore the inject option with ClassProvider and ValueProvider, right? So it's not like it should cause any runtime problems, and would really just mean someone isn't reading what Typescript is saying.

[kamilmysliwiec]

No implications at all (runtime-wise), that's correct!

[delucca]

There is already a proposition to handle such behavior:

• Issue: TypeScript#23689
• PR: TypeScript#40468

The PR idea is pretty good! But, personally, I would keep never and wait for that PR to be merged 😄 Just to avoid being "hacky" and stumbling upon any strange corner case/security issue