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(webpack): remove deprecations introduced in webpack 5 #1047

Closed
wants to merge 2 commits into from

Conversation

Delagen
Copy link

@Delagen Delagen commented Dec 27, 2019

No description provided.

@johnnyreilly
Copy link
Member

Thanks for the PR! Please could you provide some details covering what this is about / designed to achieve? It looks like this will make ts-loader only support webpack 5. Is that right?

@Delagen
Copy link
Author

Delagen commented Dec 27, 2019

@johnnyreilly As i see in ci test, it completes, and fails in uncertain circumstances. So webpack 4 compatibility kept.
I only rewrote reduce to forEach, as Set object does not support it. And use getErrors method if exists instead of errors property.

@johnnyreilly
Copy link
Member

johnnyreilly commented Dec 27, 2019

it completes, and fails in uncertain circumstances

It seems to be failing consistently when comparison tests are run. A little background:

ts-loader has 2 test packs: execution tests (which run ts-loader and see if the result of invoking ts-loader results in passing tests) and comparison tests (which run ts-loader and test the output of ts-loader in comparison to known outputs which are part of the test pack).

Execution tests run as part of all test runs and are reliable. Comparison tests run only against the latest TypeScript version in the test pack (v3.6.3 in this instance; I need to update it) and are more brittle.

More can be read about the comparison test pack here: https://github.com/TypeStrong/ts-loader/blob/master/test/comparison-tests/README.md

It looks like the comparison tests are reliably failing. If you run yarn build && yarn comparison-tests what do you experience locally?

Also, it's still not clear to me what this PR is intended to do. Would you be able to provide a brief explanation please? It does seem like it may be breaking webpack 4 compatibility, is that intentional?

@Delagen
Copy link
Author

Delagen commented Dec 30, 2019

Webpack typings does not updated yet, so ts-ignore comments used. It only remove warnings of deprecations on use in webpack 5 (Compilation.modules now is Set, so no reduce method, instead of Dependency.errors method getErrors offerred)

@johnnyreilly
Copy link
Member

Webpack typings does not updated yet,

Send a PR to definitely typed to fix this. Mention me in the PR and I'll review it for you. Let's get that fixed rather than work around it.

@Delagen
Copy link
Author

Delagen commented Dec 30, 2019

@johnnyreilly I think we need to wait to release, this changes does not prevent plugin to work)

@johnnyreilly
Copy link
Member

johnnyreilly commented Dec 30, 2019

I think we need to wait to release

For the webpack type definitions to be released? Once the PR is merged the release of them is automated. I can help there; I'm one of the maintainers of definitely typed.

Or did you mean webpack 5? I might be misunderstanding you

@Delagen
Copy link
Author

Delagen commented Dec 30, 2019 via email

@johnnyreilly
Copy link
Member

It's possibly worth continuing with this. The fact that webpack 5 is in beta should mean the API is pretty stable. Either way, at least initially we'll be looking to support both webpack 4 and webpack 5 with ts-loader. But no worries if you'd rather wait 😄

@Delagen
Copy link
Author

Delagen commented Jan 13, 2020

[DEP_WEBPACK_DEPRECATION_ARRAY_TO_SET] DeprecationWarning: Compilation.modules was changed from Array to Set (using Array method 'reduce' is deprecated)
[DEP_WEBPACK_MODULE_ERRORS] DeprecationWarning: Module.errors was removed (use getErrors instead)

@Lisiadito
Copy link

I would love to see these warnings fixed

@stale
Copy link

stale bot commented May 2, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label May 2, 2020
@stale
Copy link

stale bot commented May 9, 2020

Closing as stale. Please reopen if you'd like to work on this further.

@stale stale bot closed this May 9, 2020
@riarheos
Copy link

riarheos commented Sep 4, 2020

So how do we deal with this? Maybe reopen and apply the patch?

@riarheos
Copy link

riarheos commented Sep 4, 2020

Ah, looks like it was fixed between 8.0.0 and 8.0.3. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants