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 #18 #81

Closed
wants to merge 2 commits into from
Closed

Fix #18 #81

wants to merge 2 commits into from

Conversation

nemchik
Copy link
Contributor

@nemchik nemchik commented Jan 1, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: link tbd.
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] tooling / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

.d.ts files are included in webpack

Issue Number: #18

What is the new behavior?

.d.ts files are not included in webpack

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Other information

@nemchik
Copy link
Contributor Author

nemchik commented Jan 1, 2021

Forgot I need a changelog entry. Not sure the cadence for adding changes for a new version (will have a look at the commit history of the changelog).

@jantimon
Copy link
Contributor

jantimon commented Jan 4, 2021

This is a breaking change as it will break all apps which use .d.ts files

@nemchik
Copy link
Contributor Author

nemchik commented Jan 4, 2021

This is a breaking change as it will break all apps which use .d.ts files

I admit I am fairly new to typescript. What would be the purpose of including the .d.ts files in a webpack bundle? The readme here says the plugin configs are based on ts-loader best practices which as far as I can tell do not include the .d.ts files in the bundle.

@nemchik
Copy link
Contributor Author

nemchik commented Jan 4, 2021

I also now realize i tagged this as fixing the wrong issue. It should be #18 . I will rebase to reword the commit and edit the PR title. My apologies. (too many tabs open)

@nemchik nemchik changed the title Fix #68 Fix #18 Jan 4, 2021
@nemchik
Copy link
Contributor Author

nemchik commented Jan 4, 2021

@jantimon I had a look at the commit history for the changelog and it really looks like the only changes to the master branch are releases. If this PR were to be accepted should I aim for this PR to be considered a point release? Or is the branch strategy to merge from PRs into a staged branch that gets merged into master as a release at a later time?

@nemchik
Copy link
Contributor Author

nemchik commented Apr 7, 2021

TypeStrong/ts-loader#1277 removed the example used as the base for the configuration in ts-config-webpack-plugin. All of their current examples do not include .d.ts files. It seems all of their current examples also don't use the cache-loader or thread-loader, but that's outside the scope of this PR.

Anyway I'm still struggling to see the validity of having definition files included in the webpack bundle. I'd really like to learn what the reason for this would be.

@ernscht ernscht added the ts-config ts-config-webpack-plugin label Apr 21, 2021
@ernscht ernscht mentioned this pull request Sep 10, 2021
3 tasks
@nemchik nemchik closed this Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
check ts-config ts-config-webpack-plugin
Development

Successfully merging this pull request may close these issues.

None yet

3 participants