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

Type does not follow workspace since 4.3.2 #44488

Closed
bodinsamuel opened this issue Jun 7, 2021 · 9 comments
Closed

Type does not follow workspace since 4.3.2 #44488

bodinsamuel opened this issue Jun 7, 2021 · 9 comments
Assignees
Labels
Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@bodinsamuel
Copy link

Bug Report

Hello team,
Not sure if it's a limitation or a bug, but when using types across workspaces, some of them are not following along.
I can not explain why, but I have a small repro

🔎 Search Terms

types, express, workspaces, references,

🕗 Version & Regression Information

  • This changed between versions 4.2.4 -> 4.3.2
  • Still happening with @next

⏯ Playground Link

git clone https://github.com/bodinsamuel/ts-workspace-typing-issue.git
cd ts-workspace-typing-issue
yarn
yarn tsc -b
$ /Users/samuelbodin/code/test-ts-workspace/node_modules/.bin/tsc -b
test2/index.ts:6:17 - error TS7006: Parameter 'req' implicitly has an 'any' type.

6     'foo.Txt': (req, res) => {
                  ~~~

test2/index.ts:6:22 - error TS7006: Parameter 'res' implicitly has an 'any' type.

6     'foo.Txt': (req, res) => {

🙁 Actual behavior

Express req and res are typed as any
Screen Shot 2021-06-07 at 21 02 29

🙂 Expected behavior

Correct express typing

@bodinsamuel bodinsamuel changed the title Type does not follow workspace Type does not follow workspace since 4.3.2 Jun 7, 2021
@RyanCavanaugh
Copy link
Member

Condensed as far as I felt needed, this is a 4.2 -> 4.3 regression

type StringFunc = (a: string) => void;

declare function startHttpServer(x: { uses: Record<string, NumberFunc | StringFunc>; }): void;

export interface BoolFunc {
  (req: boolean): void;
}

export interface NumberFunc extends BoolFunc {
  (req: number): void;
}

startHttpServer({
  uses: {
    'foo.Txt': (req) => {
      
    }
  }
})

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jun 10, 2021
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.4.0 (Beta) milestone Jun 10, 2021
@bodinsamuel
Copy link
Author

Amazing @RyanCavanaugh !
So no relation to workspaces actually? :D

@orta
Copy link
Contributor

orta commented Jun 15, 2021

Shrunk it down even further, seems to be specific to the contextual typing when there's a union of functions in that position

type StringFunc = (a: string) => void;

declare function startHttpServer(x: { uses: Record<string, NumberFunc | StringFunc>; }): void;

export interface BoolFunc {
  (req: boolean): void;
}

export interface NumberFunc extends BoolFunc {
  (req: number): void;
}

startHttpServer({
  uses: {
    'foo.Txt': (req) => {
      
    }
  }
})

@typescript-bot typescript-bot added the Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros label Jun 15, 2021
@RyanCavanaugh
Copy link
Member

@orta that's not a regression, though - longstanding behavior

@orta
Copy link
Contributor

orta commented Jun 15, 2021

This seems off.

It feels like previously working before is a bug though, on 4.2 it the contextual arg is a string but it 'ideally' should be string | number | boolean in your example, there's nothing which should narrow it down to the single union participant of arg:string => void except that two members are overloads of the same func

Screen Shot 2021-06-15 at 5 43 09 PM

The 4.3 changes might have just brought it inline with the rest of the type system?

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 16, 2021

👋 Hi, I'm the Repro bot. I can help narrow down and track compiler bugs across releases! This comment reflects the current state of the repro in this issue running against the nightly TypeScript.


Comment by @orta

❌ Failed: -

  • Parameter 'req' implicitly has an 'any' type.

Historical Information

Comment by @orta

Version Reproduction Outputs
3.9.2, 4.0.2, 4.1.2, 4.2.2, 4.3.2, Nightly

❌ Failed: -

  • Parameter 'req' implicitly has an 'any' type.

@bodinsamuel
Copy link
Author

Hi,

My opinion is probably not highly relevant because I lack a lot of information here, and I'm not sure wether it's a bug or a correction. But the fact the this behavior changes between 2 minors is maybe an issue in itself.
I'm fine with breaking cahnges between major, especially if it's documented why and how to solve it,
but here it's a clear regression between 4.2.4 -> 4.3.2 (on my repro).
If it really need to happen, would be better to introduce it in 5.0.0 imo.

Best Regards,

@orta orta added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Bug A bug in TypeScript labels Jun 29, 2021
@orta
Copy link
Contributor

orta commented Jun 29, 2021

Both Major and Minors are effectively major versions for TypeScript, see for the general reasoning #14116 (comment) which is why you see long release notes for every major/minor combo.

I chatted with one of the engineers who I figured might have made the changes for this and they said this change is intentional, ignoring the other functions in the type system because they had an overload was a bug fixed in #42620

@orta orta closed this as completed Jun 29, 2021
@bodinsamuel
Copy link
Author

Make sense for the majors thanks.


For the rest I'm not sure I agree with the fix then. In the repro provided by you, the req become any while it should be perfectly okay to be number | boolean, it's not string or object or whatever, I'm not just sure why it becomes any.

More than that, I can not help the typing, if I put (req: number) the function now complain that Type 'boolean' is not assignable to type 'number', which is fair warning but a bit annoying.
You will say I need to check the typing with a type guard, which is fair too but with number | boolean a simple if req === true) would have restrained the type very closely to the actual type, while any is hardly doable.
In the end users will do req as boolean | number and now the type is no longer in sync with the initial definition. So tomorrow the definition change to string | number and typescript will not complain.
Playground

Anyway, in the end if it's to align the typing system, I have no say in this.
However I would have loved a union or unknown more than any. any let you do a lot of mistake, while unknown at least complain a bit more. I guess a even better type would be unguessable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants