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): remove faulty tsconfig.json include of test dir #646

Merged
merged 1 commit into from
Mar 28, 2020

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Mar 27, 2020

  • include means all files that should be compiled, and the test dir
    should definitely not be compiled

    • this was causing issues where declarations were being output into
      dist/ for all files in test/
      • that shouldn't happen and it means build was unnecessarily
        slowed down in order to process test/ files
      • default exclude that was added of .spec/.test files,
        originally for co-located tests, ended up making most test/ dir
        files excluded too, but not, say, test utils that don't have a
        .spec or .test suffix
  • this was also causing errors with the rootDir: './src' change as
    the test/ dir is outside of the rootDir

    • problem wasn't the rootDir, but that the test/ dir shouldn't be
      compiled or processed in any way

(test): ensure test/.test.ts and test/.ts files are correctly
excluded and don't have declarations generated

  • and that they don't error with rootDir: './src'

(test): ensure types/*.d.ts don't error with rootDir and don't have
declarations re-generated either

  • n.b. tsc won't re-generate them either, they don't get copied to
    outDir, this isn't new or different behavior

(env/test): Jest shouldn't run / should ignore tests inside of fixtures


Fixes #638 , the root cause of it, and adds tests for this.

Effectively reverts #226 . I haven't been able to reproduce #225 / #84 in my VS Code, but maybe that's because I have it configured a little differently?
Would like to add VS Code integration tests so there's an automated way to check that it works independently on CI without any configuration (and so I don't have to attempt and fail to test this on my own machine).
But tsc --noEmit at least, indeed doesn't output errors in test/ anymore. tsdx lint still does.
Per my comments in #638 if this and the VS Code integration are problematic, we should move this to a tsconfig.build.json and then have a plain tsconfig.json that extends from it and changes include to './' so everything is type-checked.

@agilgur5 agilgur5 force-pushed the remove-include-test branch 2 times, most recently from 5776f7f to 6aeda02 Compare March 27, 2020 20:15
Copy link
Collaborator Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a bit of changes to the tests.
Otherwise, I iterated on the comments a few times and split off #648 so those look good now.
Actual changes are quite small.

test/e2e/tsdx-build-default.test.js Outdated Show resolved Hide resolved
- `include` means all files that should be compiled, and the test dir
  should definitely not be compiled
  - this was causing issues where declarations were being output into
    dist/ for all files in test/
    - that shouldn't happen and it means `build` was unnecessarily
      slowed down in order to process test/ files
    - default `exclude` that was added of *.spec/*.test files,
      originally for co-located tests, ended up making most test/ dir
      files excluded too, but not, say, test utils that don't have a
      .spec or .test suffix

- this was also causing errors with the `rootDir: './src'` change as
  the test/ dir is outside of the rootDir
  - problem wasn't the rootDir, but that the test/ dir shouldn't be
    compiled or processed in any way
  - add a note about this to the moveTypes() deprecation warning

(test): ensure test/*.test.ts and test/*.ts files are correctly
excluded and don't have declarations generated
- and that they don't error with rootDir: './src'

(test): ensure types/*.d.ts don't error with rootDir and don't have
declarations re-generated either
- n.b. tsc won't re-generate them either, they don't get copied to
  outDir, this isn't new or different behavior

(env/test): Jest shouldn't run / should ignore tests inside of fixtures
@agilgur5 agilgur5 force-pushed the remove-include-test branch from 6aeda02 to 2195e90 Compare March 28, 2020 23:10
Copy link
Collaborator Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now. Considered adding backticks to the deprecation warning and making it yellow, but don't want it to be too in your face yet as we're still allowing it to be used for the foreseeable future.
Before we decide to remove it we should make it yellow and state it will be removed in a future update in the warning itself.

@kylemh
Copy link
Contributor

kylemh commented Apr 4, 2020

Does this mean we'd also wanna avoid compiling stories (Storybook)?

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Apr 4, 2020

@kylemh yes, your stories shouldn't be in your include, otherwise they'll be unnecessarily compiled (and output to dist/likely unnecessarily published).

The Storybook template doesn't have them in the include as you can see here. Are yours by chance co-located? We could add **/*.stories.tsx to the default exclude if it's a common enough pattern (I've seen it a few times).

The approach I listed in #638 (comment), especially with file: ['src/index.ts'] is optimal, but unfortunately seems to have issues in upstream rpts2 as a I mentioned there.

@kylemh
Copy link
Contributor

kylemh commented Apr 4, 2020

So I’m doing monorepo shit in a personal project first before I try to come up with a template to resolve #122

https://github.com/innocuous-tech/core

I’ve got it all flying for now, but I can’t see how to prevent the stories’ typings from getting compiled into the bundle.

I have it excluded, but it’s not working... what do you think?

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Apr 4, 2020

Ah yours are like semi-colocated, in the src dir but as separate dirs. I'd leave them outside of src for a template fwiw.
Probably just need to mess with the globs a bit. If you moved the exclude into each package it would probably be easier to figure out. Path resolution is a bit weird with extends too but you seem to be using it correctly so I'm not sure.

@kylemh
Copy link
Contributor

kylemh commented Apr 4, 2020

Hmmmm I remember reading somewhere that @jaredpalmer prefers colocating tests with source code. Having the answer be “don’t do that” doesn’t seem ideal or in line with the opinions of this repo’s author.

I’ll try manually listing the excludes in each package, but that wasn’t working either.

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Apr 4, 2020

Well none of the existing templates have co-located tests or stories, so it wouldn't make sense to add a monorepo template that is the odd one out with semi-colocated tests. Would be very inconsistent and confusing to users

@kylemh
Copy link
Contributor

kylemh commented Apr 4, 2020

So this is my individual use case. I can make the template match the rest. The point I’m making is that we should be able to have collocated tests and stories.

In my example, test files are ignored but not stories, so I was just tryna make sure it wasn’t something under the hood. Not extending and excluding per package seems to be working. I’ll have to figure out where I’m going wrong with extends

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Apr 4, 2020

The include/exclude handling should work the same as tsc, the only difference is that TSDX has a default exclude for fully co-located tests (if you're adding your own it doesn't matter). Though it's possible there are bugs with rpts2 as that's one reason why I haven't moved templates to just a files set-up which wouldn't be impacted by location at all, only by imports.

paul-vd pushed a commit to EezyQuote/tsdx that referenced this pull request Dec 1, 2020
- `include` means all files that should be compiled, and the test dir
  should definitely not be compiled
  - this was causing issues where declarations were being output into
    dist/ for all files in test/
    - that shouldn't happen and it means `build` was unnecessarily
      slowed down in order to process test/ files
    - default `exclude` that was added of *.spec/*.test files,
      originally for co-located tests, ended up making most test/ dir
      files excluded too, but not, say, test utils that don't have a
      .spec or .test suffix

- this was also causing errors with the `rootDir: './src'` change as
  the test/ dir is outside of the rootDir
  - problem wasn't the rootDir, but that the test/ dir shouldn't be
    compiled or processed in any way
  - add a note about this to the moveTypes() deprecation warning

(test): ensure test/*.test.ts and test/*.ts files are correctly
excluded and don't have declarations generated
- and that they don't error with rootDir: './src'

(test): ensure types/*.d.ts don't error with rootDir and don't have
declarations re-generated either
- n.b. tsc won't re-generate them either, they don't get copied to
  outDir, this isn't new or different behavior

(env/test): Jest shouldn't run / should ignore tests inside of fixtures
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.

test/ dir is in include, non-spec TS files (e.g. test utils) break build with v0.13 rootDir: ./src
2 participants