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: detect type for URLs with query parameter or fragment identifier #3509

Merged
merged 1 commit into from
May 16, 2020

Conversation

devoto13
Copy link
Collaborator

Closes #3497

log.warn(`Invalid file type (${fileType}), defaulting to js.`)
const fileType = file.type || file.detectType()

if (!FILE_TYPES.includes(fileType)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

helper.isDefined(fileType) always returns true with current code, that's why it was removed.

@karmarunnerbot
Copy link
Member

Build karma 270 completed (commit ac75a7ed94 by @devoto13)

@devoto13 devoto13 requested a review from johnjbarton May 14, 2020 22:18
@karmarunnerbot
Copy link
Member

Build karma 269 completed (commit 27394ff3a6 by @devoto13)

lib/file.js Outdated Show resolved Hide resolved
lib/url.js Outdated Show resolved Hide resolved

if (!FILE_TYPES.includes(fileType)) {
log.warn(
`Invalid file type (${fileType}), defaulting to js.\n` +
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ($fileType || 'filename has no .extension')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if it really makes the warning more understandable... Before it was simply saying that $fileType is an invalid file type, but now it also introduces a relation between file type and file extension without really explaining it.

How about having different messages depending on whether it was an invalid value in the configuration (this should probably be changed to an error, but in a separate PR) vs Karma failed to guess a valid type from the file extension? See updated PR.

My logic with the error message is to say:

  • what happened? - Karma failed to guess a file type
  • how to fix it? - specify file type in the configuration file
  • don't understand what is it about? - read the documentation link that explains it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And my assumption here is that normally people want to specify the type in the configuration, not change the extension of the file. E.g. testing .vue single file components or #3491 (comment).

@karmarunnerbot
Copy link
Member

Build karma 271 completed (commit 6e880ae589 by @devoto13)

@karmarunnerbot
Copy link
Member

Build karma 270 completed (commit 6e880ae589 by @devoto13)

` See http://karma-runner.github.io/latest/config/files.html`
)
} else {
log.warn(`Invalid file type (${file.type}), defaulting to js.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

I just think the message may be confusing in the case file.type === '':
Invalid file type (), defaulting to js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be Invalid file type (empty string), defaulting to js. now.

@karmarunnerbot
Copy link
Member

Build karma 276 completed (commit a0e502a4f3 by @devoto13)

@karmarunnerbot
Copy link
Member

Build karma 275 completed (commit a0e502a4f3 by @devoto13)

@johnjbarton johnjbarton merged commit f399063 into karma-runner:master May 16, 2020
karmarunnerbot pushed a commit that referenced this pull request May 16, 2020
## [5.0.7](v5.0.6...v5.0.7) (2020-05-16)

### Bug Fixes

* detect type for URLs with query parameter or fragment identifier ([#3509](#3509)) ([f399063](f399063)), closes [#3497](#3497)
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 5.0.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@devoto13 devoto13 deleted the detect-url-type branch May 17, 2020 11:40
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
## [5.0.7](karma-runner/karma@v5.0.6...v5.0.7) (2020-05-16)

### Bug Fixes

* detect type for URLs with query parameter or fragment identifier ([karma-runner#3509](karma-runner#3509)) ([f399063](karma-runner@f399063)), closes [karma-runner#3497](karma-runner#3497)
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.

3 participants