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

Ensure co-located test files don't have definitions generated #472

Merged
merged 6 commits into from
Feb 3, 2020

Conversation

selbekk
Copy link
Contributor

@selbekk selbekk commented Jan 30, 2020

This commit fixes an issue where type declaration files were created
for test files as well.

Fixes #464

This commit fixes an issue where type declaration files were created
for test files as well.
Copy link
Collaborator

@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.

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:

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 include src/ though (vs. TSDX defaults can be against any include)
  • 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/ (the include directory in general); the TSDX convention of using a test/ dir will not encounter this issue

@selbekk selbekk changed the title Exclude test files from typescript typings Ensure co-located test files don't have definitions generated Jan 31, 2020
@selbekk
Copy link
Contributor Author

selbekk commented Jan 31, 2020

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?

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 1, 2020

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 test/ directories either. In templates that means people with test/ would have to delete the excludes (or leave them in redundantly); some might also not understand that they're not necessarily needed for all projects.

In any case, let's wait for some thoughts from the other maintainers as they might decline wholesale these changes, idk

Copy link
Collaborator

@swyxio swyxio left a 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

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 2, 2020

@sw-yx any thoughts on adding this to templates vs. adding it to tsconfigDefaults (of rpts2)?

@swyxio
Copy link
Collaborator

swyxio commented Feb 3, 2020

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.

@selbekk
Copy link
Contributor Author

selbekk commented Feb 3, 2020

If we add it to the tsconfigDefaults, we can fix this issue for both existing users of tsdx and new ones.

@selbekk
Copy link
Contributor Author

selbekk commented Feb 3, 2020

I'm not sure why this isn't working on the windows-latest build - should work as expected?

Copy link
Collaborator

@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.

Some tiny changes:

  • add some comments
  • paths.appDist is used internally

src/createRollupConfig.ts Outdated Show resolved Hide resolved
src/createRollupConfig.ts Show resolved Hide resolved
src/createRollupConfig.ts Show resolved Hide resolved
selbekk and others added 3 commits February 3, 2020 22:00
Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>
Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>
Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>
@swyxio swyxio merged commit a56894f into jaredpalmer:master Feb 3, 2020
@swyxio
Copy link
Collaborator

swyxio commented Feb 3, 2020

@all-contributors pls thank @selbekk for code

@allcontributors
Copy link
Contributor

@sw-yx

I could not determine your intention.

Basic usage: @all-contributors please add @Someone for code, doc and infra

For other usages see the documentation

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 3, 2020

@all-contributors please add @selbekk for code

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @selbekk! 🎉

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.

Co-located test files have type declarations generated in dist folder
3 participants