-
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
Improve file type detection #3497
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
✅ Build karma 2648 completed (commit a12d56d15d by @falsandtru) |
✅ Build karma 250 completed (commit a12d56d15d by @falsandtru) |
✅ Build karma 249 completed (commit a12d56d15d by @falsandtru) |
Looks like a validation error of cla.
|
Thanks for the PR @falsandtru. The issue is that the commit message does not follow the repository rules. This change will also need unit tests. But before getting to it, I don't quite understand how you end up with query params added to your scripts. Can you share an example config and elaborate on your use case please? |
OK, I'll fix the commit message. My config is here. https://github.com/falsandtru/securemark/blob/master/karma.conf.js |
✅ Build karma 2649 completed (commit 7e77641f2c by @falsandtru) |
✅ Build karma 251 completed (commit 7e77641f2c by @falsandtru) |
✅ Build karma 250 completed (commit 7e77641f2c by @falsandtru) |
✅ Build karma 252 completed (commit 222c5912b0 by @falsandtru) |
✅ Build karma 251 completed (commit 222c5912b0 by @falsandtru) |
✅ Build karma 2650 completed (commit 222c5912b0 by @falsandtru) |
Can you merge? |
@falsandtru We still need unit tests to ensure future updates will not break this logic. I think the easiest way is to extract Instructions how to make changes to the repository can be found here. |
✅ Build karma 263 completed (commit 70989c2f70 by @falsandtru) |
done |
✅ Build karma 262 completed (commit 70989c2f70 by @falsandtru) |
✅ Build karma 2661 completed (commit 70989c2f70 by @falsandtru) |
@@ -141,6 +141,10 @@ const replaceWinPath = (path) => { | |||
|
|||
exports.normalizeWinPath = process.platform === 'win32' ? replaceWinPath : _.identity | |||
|
|||
exports.getFileType = (file) => { | |||
return file.type || path.extname(file.path.split(/[?#]/, 1)[0] || '').substring(1) |
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.
Is there any specific reason why we need || ''
here?
Otherwise LGTM.
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.
Also it may be better to first take extensions and then split as file name may contain ?
or #
(as it was before). E.g. myfile?.js
or myfile#.js
are valid filenames on Linux.
Hm, actually path.extname('http://example.com/mylib.js?load=test.md')
will fail and result in md
, not js
. Probably we need a more robust logic here...
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.
Is there any specific reason why we need || '' here?
String#split method can return an empty array.
Probably we need a more robust logic here...
Yes. I'm expecting that someone write a stricter implementation later.
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.
Hm, looks like the source code treats paths only as URLs.
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.
Yes. I'm expecting that someone write a stricter implementation later.
Well, current implementation breaks file type detection for myfile?.js
and myfile#.js
in favour of URLs. I don't think it is appropriate to break one corner case in favour of another.
Here is a better idea. Instead of having single method to handle both file path and URL we move the file type detection to constructors in file.js
and url.js
correspondingly. This way we can provide appropriate implementation depending on what we're dealing with.
E.g.
path.extname('src/myfile.js').substring(1)
for file paths
path.extname(new URL('https://example.com/myfile.js?test=param').pathname).substring(1)
for URLs
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. You should do such refactoring yourself.
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.
Okey, I'll try and submit an alternative PR in the coming days.
Maybe also change commit message to something like "detect file type for URLs with query params and fragment identifiers", so it is more clear what exactly is fixed. |
I got an error.
|
✅ Build karma 264 completed (commit 7f96c568ed by @falsandtru) |
✅ Build karma 2663 completed (commit 454f1eac29 by @falsandtru) |
✅ Build karma 265 completed (commit 454f1eac29 by @falsandtru) |
✅ Build karma 264 completed (commit c5d5e7d4c8 by @falsandtru) |
✅ Build karma 2664 completed (commit 1c0fabc3b1 by @falsandtru) |
✅ Build karma 266 completed (commit 1c0fabc3b1 by @falsandtru) |
✅ Build karma 265 completed (commit 1c0fabc3b1 by @falsandtru) |
🎉 This issue has been resolved 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)
Suppress verbose warnings.