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(concatjs): update karma to 6.3.2 and fix #2093 #2603

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

mattsoulanille
Copy link
Contributor

@mattsoulanille mattsoulanille commented Apr 13, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
    No new tests added, but one was fixed to work with the change.
  • Docs have been added / updated (for bug fixes / features)
    Docs shouldn't need to be updated for this change.

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #2093

What is the new behavior?

Fixes #2093. Updates karma packages to latest versions.

Does this PR introduce a breaking change?

  • Yes
  • No

The old version of karma seems to still work with these changes (n = 2 tests), but I haven't extensively tested it.

Other information

@google-cla google-cla bot added the cla: yes label Apr 13, 2021
@@ -4,6 +4,7 @@
*/
import * as crypto from 'crypto';
import * as fs from 'fs';
import * as File from 'karma/lib/file';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is somewhat hacky. We're probably not supposed to import from lib

@mattsoulanille mattsoulanille marked this pull request as ready for review April 13, 2021 02:36
@mattem
Copy link
Collaborator

mattem commented Apr 13, 2021

Should we be bumping the peer dep here: https://github.com/bazelbuild/rules_nodejs/blob/stable/packages/concatjs/package.json#L25 ? Or this this API available in 4.0.0 too

@mattsoulanille
Copy link
Contributor Author

Nice catch. I forgot that there are separate package.json files for the pkg_npm targets.

We might not actually have to bump the peer dependency. The only key missing in the new File object that is present in the current implementation's object is content, which the current one sets to the empty string. Karma 4 seems to work fine without it, at least with our tests in //packages/concatjs/web_test/test/karma_typescript/... and //packages/concatjs/web_test/test/karma/....

We could still bump it to 5.1.0 just in case. What do you think?

@mattem
Copy link
Collaborator

mattem commented Apr 19, 2021

As the tests pass I think this shows it's backwards compatible - we could bump the version as part of the upcoming 4.0.0 milestone release if we wanted, then it's a clear breaking point.

@mattsoulanille
Copy link
Contributor Author

Sounds good to me. Thanks!

@mattem mattem merged commit c80479d into bazel-contrib:stable Apr 19, 2021
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.

[karma-server]: UncaughtException:: file.detectType is not a function
2 participants