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: restore customFileHandlers provider #3624

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

devoto13
Copy link
Collaborator

@devoto13 devoto13 commented Jan 19, 2021

The removal of customFileHandlers turned out to be more disruptive change than anticipated as this provider is still used by several popular plugins: #3619, codymikol/karma-webpack#462, angular/angular-cli#19815. Hence we restore the provider and print a deprecation warning to make upgrading easier. This should give more time for the plugin authors to release new versions and users to adopt these versions.

How it looks now:

image

@karmarunnerbot
Copy link
Member

Build karma 468 failed (commit bc09f16a3e by @devoto13)

@karmarunnerbot
Copy link
Member

Build karma 467 failed (commit bc09f16a3e by @devoto13)

return function (request, response, next) {
const handler = customFileHandlers.find((handler) => handler.urlRegex.test(request.url))

if (customFileHandlers.length > 0 && !warningDone) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The awkward logic is here because handlers are added dynamically (e.g. https://github.com/angular/angular-cli/pull/19784/files#diff-7e99b8c9ae4a43ae87d525fccfd19e3c7449e3f12b3d0e07aedf208a8a451f3cL223) and we can't rely on the presence of the provider either, because it is always there.

So I resorted to checking that there is at least one custom handler is present when handling a request and printing the warning only once to avoid spamming the logs.

The check is not perfect as the warning is printed only when this handler is reached but this should be good enough in practice.

@alan-agius4
Copy link
Contributor

alan-agius4 commented Jan 19, 2021

JFYI, Angular CLI 11.0 doesn't support Karma 6, thus the error is expected. Karma 6 is supported in Angular 11.1. See: angular/angular-cli#19815 (comment)

@karmarunnerbot
Copy link
Member

Build karma 469 completed (commit e5ae8e586a by @devoto13)

@devoto13
Copy link
Collaborator Author

@alan-agius4 Yes, I know. But people still upgrade to Karma 6 and get the error. I think it is better to have the warning to give the ecosystem time to adapt. This also affects older versions of karma-webpack plugin (and there is no stable version with the fix). Also some people want to upgrade to fix some CVEs (#3619 (comment)).

@karmarunnerbot
Copy link
Member

Build karma 468 completed (commit e5ae8e586a by @devoto13)

@alan-agius4
Copy link
Contributor

@devoto13, Agreed!

Although warnings as typically ignored. Strictly speaking in Angular CLI, they already get a warning of unmeet peer dependencies.

@SymbioticKilla
Copy link
Contributor

@devoto13 Thank you very much! Really a good decision.

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need an Issues: Release 7 blockers to remember to follow through.

test/unit/web-server.spec.js Outdated Show resolved Hide resolved
The removal of `customFileHandlers` turned out to be more disruptive change than anticipated as this provider is still used by several popular plugins: karma-runner#3619, codymikol/karma-webpack#462, angular/angular-cli#19815. Hence we restore the provider and print a deprecation warning to make upgrading easier. This should give more time for the plugin authors to release new versions and users to adopt these versions.
@karmarunnerbot
Copy link
Member

Build karma 472 completed (commit a89096a703 by @devoto13)

@karmarunnerbot
Copy link
Member

Build karma 471 completed (commit a89096a703 by @devoto13)

@johnjbarton johnjbarton merged commit 25d9abb into karma-runner:master Jan 20, 2021
karmarunnerbot pushed a commit that referenced this pull request Jan 20, 2021
## [6.0.1](v6.0.0...v6.0.1) (2021-01-20)

### Bug Fixes

* **server:** set maxHttpBufferSize to the socket.io v2 default ([#3626](#3626)) ([69baddc](69baddc)), closes [#3621](#3621)
* restore `customFileHandlers` provider ([#3624](#3624)) ([25d9abb](25d9abb))
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 6.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@devoto13 devoto13 deleted the restore-custom-files branch February 4, 2021 18:18
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
The removal of `customFileHandlers` turned out to be more disruptive change than anticipated as this provider is still used by several popular plugins: karma-runner#3619, codymikol/karma-webpack#462, angular/angular-cli#19815. Hence we restore the provider and print a deprecation warning to make upgrading easier. This should give more time for the plugin authors to release new versions and users to adopt these versions.
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
## [6.0.1](karma-runner/karma@v6.0.0...v6.0.1) (2021-01-20)

### Bug Fixes

* **server:** set maxHttpBufferSize to the socket.io v2 default ([karma-runner#3626](karma-runner#3626)) ([69baddc](karma-runner@69baddc)), closes [karma-runner#3621](karma-runner#3621)
* restore `customFileHandlers` provider ([karma-runner#3624](karma-runner#3624)) ([25d9abb](karma-runner@25d9abb))
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.

5 participants