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

Let the compiler shout at me on typeerrors: #39

Closed
NobbZ opened this issue Jun 17, 2017 · 8 comments · Fixed by #40
Closed

Let the compiler shout at me on typeerrors: #39

NobbZ opened this issue Jun 17, 2017 · 8 comments · Fixed by #40

Comments

@NobbZ
Copy link
Member

NobbZ commented Jun 17, 2017

I expect the following snippet to barf at me before even trying to execute the tests:

class HelloWorld {
    static hello(name = "World" : string): number {
        return "Hello, " + name + "!"
    }
}

export default HelloWorld

Instead of barfing, yarn test && yarn lint just complains about string beeing a variable name which clashes with a built in type:

yarn test v0.22.0
$ jest --no-cache
 PASS  ./hello-world.test.ts
  Hello World
    ✓ says hello world with no name (2ms)
    ✓ says hello to bob (1ms)
    ✓ says hello to sally

Test Suites: 1 passed, 1 total
Tests:       3 passed, 3 total
Snapshots:   0 total
Time:        1.063s
Ran all test suites.
Done in 1.79s.
yarn lint v0.22.0
$ tslint '*.ts?(x)'; exit 0

hello-world.ts[2, 35]: variable name clashes with keyword/type
Done in 0.48s.

So currently, in how the tests are run, it seems as if typescript is just another typeless hell around javascript. Therefore the request to do the following:

  • Make the compiler fail on type errors (absolute minimum)
  • Make the compiler (or the linter) warn or error (the former is totally enough I think) on missing types
@NobbZ
Copy link
Member Author

NobbZ commented Jun 17, 2017

PS: totally the same behaviour when s/string/number/

@NobbZ
Copy link
Member Author

NobbZ commented Jun 17, 2017

OK, in the meantime I was able to find out, that tsc will complain about a syntax error:

hello-world.ts(2,33): error TS1005: ',' expected.

After some googling I found that correct syntax for the class were:

class HelloWorld {
    static hello(name: number = "World"): number {
        return "Hello, " + name + "!"
    }
}

compiling this, now successfully complains about types beeing incorrect:

hello-world.ts(2,18): error TS2322: Type '"World"' is not assignable to type 'number'.
hello-world.ts(3,9): error TS2322: Type 'string' is not assignable to type 'number'.
hello-world.test.ts(11,29): error TS2345: Argument of type '"Bob"' is not assignable to parameter of type 'number'.
hello-world.test.ts(15,29): error TS2345: Argument of type '"Sally"' is not assignable to parameter of type 'number'.

Therefore I have to assume, that the typescript-track currently doesn't even use typescript!

@masters3d
Copy link
Contributor

Good point. I filed a bug/feature question.
As a work around we could run the compiler every time we run the tests to catch this
"test": "tsc -p . && jest --no-cache",
I do think since ts-jest compiles the code before running jest that they should display any errors from the compiler and fail.

@NobbZ
Copy link
Member Author

NobbZ commented Jun 17, 2017

You filed a bug/feature where? Do you have a link/crossref?

@masters3d
Copy link
Contributor

kulshekhar/ts-jest#250

@NobbZ
Copy link
Member Author

NobbZ commented Jun 17, 2017

Also, a better way to alter the script line seems to be: tsc --noEmit -p . && jest --no-cache.

This way there is no javascript generated which leads to running the tests twice by jest

@masters3d
Copy link
Contributor

What rabbit hole. Others are also using --noEmit. I think that is what we are going to use too: ds300/react-native-typescript-transformer#13

@thedevelopnik
Copy link
Contributor

thedevelopnik commented Jun 23, 2017

@masters3d thanks for doing that PR! Looks like a good solution.

And thanks for catching this @NobbZ. Just implemented the tsc --noEmit in my own copy of the exercises and found out my Hello World solution didn't pass anymore hah.

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 a pull request may close this issue.

3 participants