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

Missing type checking in tests #797

Open
MonicaOlejniczak opened this issue Aug 3, 2021 · 3 comments
Open

Missing type checking in tests #797

MonicaOlejniczak opened this issue Aug 3, 2021 · 3 comments
Labels
developer experience 🏖️ It's something that improves the developer experience

Comments

@MonicaOlejniczak
Copy link
Collaborator

MonicaOlejniczak commented Aug 3, 2021

Describe the bug
Compiled uses TypeScript project references, which unfortunately prevents consumers from only type checking via tsc --noEmit or ttsc --noEmit. In order to determine whether the codebase has type errors, the entire project needs to be built (refer to microsoft/TypeScript#40431 and https://www.typescriptlang.org/docs/handbook/project-references.html):

tsc will not automatically build dependencies unless invoked with the --build switch

Therefore, the main type checking is performed through the yarn build command. However, test files are explicitly excluded through the shared config as seen here. This means that potential type errors and problematic cases are missed in the tests, including what is meant to be type tests (e.g. should not type error with nested arrow funcs is not properly asserted), invalid parameters (e.g. { minify: true }) are not detected, missing type dependencies, etc.

To Reproduce
Steps to reproduce the behaviour:

  1. Add the following line to a test file: const invalidNumber: number = 'not a number';
  2. Run yarn build — notice there are no errors
  3. Add the following line to a source file: const invalidBoolean: boolean = 'not a boolean';
  4. Run yarn build — notice there are now type errors

Expected behaviour
Running yarn build (or specific type checking script) should result in a type error within a test file.

Notes

  • yarn build:cjs and yarn build esm will produce different errors, as different packages are built
@MonicaOlejniczak MonicaOlejniczak added the developer experience 🏖️ It's something that improves the developer experience label Aug 3, 2021
@itsdouges
Copy link
Collaborator

itsdouges commented Aug 3, 2021

I think the current setup with Jest will error tests if there are type errors, if that gives you better confidence 🤔

Have a double check 😄, hopefully I remember correctly.

@MonicaOlejniczak
Copy link
Collaborator Author

@Madou Running yarn test packages/react/src/styled/__tests__/index.test.tsx from this:

it('should not type error', () => {
  expect(() => {
    styled.div<{ primary: string }>({
      fontSize: '20px',
      color: (props) => props.primary,
      margin: '20px',
      ':hover': {
        color: 'red',
      },
    });
  }).not.toThrow();
});

to this, which removes the generic, has no effect on the test output:

it('should not type error', () => {
  expect(() => {
    styled.div({
      fontSize: '20px',
      color: (props) => props.primary,
      margin: '20px',
      ':hover': {
        color: 'red',
      },
    });
  }).not.toThrow();
});

The expected type error is, which is produced by including the test files:

packages/react/src/styled/__tests__/index.test.tsx:271:27 - error TS2571: Object is of type 'unknown'.

271         color: (props) => props.primary,

Though my editor integration (IntelliJ) has a slightly different error:

TS7006: Parameter 'props' implicitly has an 'any' type.

@itsdouges
Copy link
Collaborator

@MonicaOlejniczak I think this might be fixed now thanks to the PR I put up refactoring the local dev loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer experience 🏖️ It's something that improves the developer experience
Projects
None yet
Development

No branches or pull requests

2 participants