-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
log.warn(`Invalid file type (${fileType}), defaulting to js.`) | ||
const fileType = file.type || file.detectType() | ||
|
||
if (!FILE_TYPES.includes(fileType)) { |
There was a problem hiding this comment.
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.
✅ Build karma 270 completed (commit ac75a7ed94 by @devoto13) |
✅ Build karma 269 completed (commit 27394ff3a6 by @devoto13) |
lib/middleware/karma.js
Outdated
|
||
if (!FILE_TYPES.includes(fileType)) { | ||
log.warn( | ||
`Invalid file type (${fileType}), defaulting to js.\n` + |
There was a problem hiding this comment.
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')
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
✅ Build karma 271 completed (commit 6e880ae589 by @devoto13) |
✅ Build karma 270 completed (commit 6e880ae589 by @devoto13) |
lib/middleware/karma.js
Outdated
` See http://karma-runner.github.io/latest/config/files.html` | ||
) | ||
} else { | ||
log.warn(`Invalid file type (${file.type}), defaulting to js.`) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
✅ Build karma 276 completed (commit a0e502a4f3 by @devoto13) |
✅ Build karma 275 completed (commit a0e502a4f3 by @devoto13) |
🎉 This PR is included in version 5.0.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [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)
Closes #3497