-
Notifications
You must be signed in to change notification settings - Fork 509
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
Ensure co-located test files don't have definitions generated #472
Conversation
This commit fixes an issue where type declaration files were created for test files as well.
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.
Thanks for making the PR!!
Oh, you just changed the templates for this -- I thought you were going to change the TSDX defaults located here:
tsdx/src/createRollupConfig.ts
Line 142 in f12fcdf
tsconfigDefaults: { |
It'll be a discussion in any case whether this change is worth merging.
A few comments/changes in any case:
- You are missing a few things from TS's defaults:
The "exclude" property defaults to excluding the node_modules, bower_components, jspm_packages and <outDir> directories when not specified.
- Why limit to
src/
folder? I guess for the templates it makes sense since they only includesrc/
though (vs. TSDX defaults can be against anyinclude
) - Could you change the title of the PR & commit to something like "Ensure co-located test files don't have definitions generated"? Currently, it's a bit misleading -- test files will only have definitions generated if they're co-located in
src/
(theinclude
directory in general); the TSDX convention of using atest/
dir will not encounter this issue
Updated the list of excludes in all templates. I can centralise in the defaults if you want - especially since the list got a bit longer? |
Thanks for the changes! In my opinion, I would think it's better to add these universally as it makes the excludes transparent for folks with project structures like yours, and makes no changes to those with In any case, let's wait for some thoughts from the other maintainers as they might decline wholesale these changes, idk |
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.
im fine with this
@sw-yx any thoughts on adding this to templates vs. adding it to |
i dont have strong feelings about this, i can see arguments for both sides. happy to go with whatever you and @selbekk want to do. |
If we add it to the tsconfigDefaults, we can fix this issue for both existing users of tsdx and new ones. |
I'm not sure why this isn't working on the windows-latest build - should work as expected? |
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.
Some tiny changes:
- add some comments
paths.appDist
is used internally
Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>
Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>
Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>
@all-contributors pls thank @selbekk for code |
I could not determine your intention. Basic usage: @all-contributors please add @Someone for code, doc and infra For other usages see the documentation |
@all-contributors please add @selbekk for code |
I've put up a pull request to add @selbekk! 🎉 |
This commit fixes an issue where type declaration files were created
for test files as well.
Fixes #464