-
Notifications
You must be signed in to change notification settings - Fork 508
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
Conversation
5776f7f
to
6aeda02
Compare
There was a problem hiding this 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.
- `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
6aeda02
to
2195e90
Compare
There was a problem hiding this 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.
Does this mean we'd also wanna avoid compiling stories (Storybook)? |
@kylemh yes, your stories shouldn't be in your The Storybook template doesn't have them in the The approach I listed in #638 (comment), especially with |
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? |
Ah yours are like semi-colocated, in the |
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. |
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 |
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 |
The |
- `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
include
means all files that should be compiled, and the test dirshould definitely not be compiled
dist/ for all files in test/
build
was unnecessarilyslowed down in order to process test/ files
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 asthe test/ dir is outside of the rootDir
compiled or processed in any way
(test): ensure test/.test.ts and test/.ts files are correctly
excluded and don't have declarations generated
(test): ensure types/*.d.ts don't error with rootDir and don't have
declarations re-generated either
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 intest/
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 plaintsconfig.json
that extends from it and changesinclude
to'./'
so everything is type-checked.