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

Improve file type detection #3497

Closed
wants to merge 2 commits into from
Closed

Conversation

falsandtru
Copy link
Contributor

@falsandtru falsandtru commented May 5, 2020

Suppress verbose warnings.

06 05 2020 07:43:55.461:WARN [middleware:karma]: Invalid file type (js?flags=gated&features=default), defaulting to js.
06 05 2020 07:43:55.463:WARN [middleware:karma]: Invalid file type (js?config=TeX-AMS_CHTML,Safe), defaulting to js.

@googlebot
Copy link

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@falsandtru
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@AppVeyorBot
Copy link

Build karma 2648 completed (commit a12d56d15d by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 250 completed (commit a12d56d15d by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 249 completed (commit a12d56d15d by @falsandtru)

@falsandtru
Copy link
Contributor Author

Looks like a validation error of cla.

The command "./scripts/validate-commit-msg.sh $COMMIT_TO_VALIDATE" exited with 1.

@devoto13
Copy link
Collaborator

devoto13 commented May 6, 2020

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?

@falsandtru
Copy link
Contributor Author

OK, I'll fix the commit message. My config is here.

https://github.com/falsandtru/securemark/blob/master/karma.conf.js

@AppVeyorBot
Copy link

Build karma 2649 completed (commit 7e77641f2c by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 251 completed (commit 7e77641f2c by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 250 completed (commit 7e77641f2c by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 252 completed (commit 222c5912b0 by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 251 completed (commit 222c5912b0 by @falsandtru)

@AppVeyorBot
Copy link

Build karma 2650 completed (commit 222c5912b0 by @falsandtru)

@falsandtru
Copy link
Contributor Author

Can you merge?

@devoto13
Copy link
Collaborator

devoto13 commented May 9, 2020

@falsandtru We still need unit tests to ensure future updates will not break this logic.

I think the easiest way is to extract detectFileType(pathOrUrl) function at the bottom of the same file and add describe('detectFileType', () => { ... }) block with the relevant tests to the test/unit/middleware/karma.spec.js file.

Instructions how to make changes to the repository can be found here.

@karmarunnerbot
Copy link
Member

Build karma 263 completed (commit 70989c2f70 by @falsandtru)

@falsandtru
Copy link
Contributor Author

done

@karmarunnerbot
Copy link
Member

Build karma 262 completed (commit 70989c2f70 by @falsandtru)

@AppVeyorBot
Copy link

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)
Copy link
Collaborator

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.

Copy link
Collaborator

@devoto13 devoto13 May 9, 2020

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...

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

@devoto13 devoto13 May 10, 2020

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@devoto13
Copy link
Collaborator

devoto13 commented May 9, 2020

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.

@falsandtru
Copy link
Contributor Author

I got an error.

input: fix(middleware): detect file type for URLs with query params and fragment identifiers
✖ header must not be longer than 72 characters, current length is 85 [header-max-length]

@karmarunnerbot
Copy link
Member

Build karma 264 completed (commit 7f96c568ed by @falsandtru)

@AppVeyorBot
Copy link

Build karma 2663 completed (commit 454f1eac29 by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 265 completed (commit 454f1eac29 by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 264 completed (commit c5d5e7d4c8 by @falsandtru)

@AppVeyorBot
Copy link

Build karma 2664 completed (commit 1c0fabc3b1 by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 266 completed (commit 1c0fabc3b1 by @falsandtru)

@karmarunnerbot
Copy link
Member

Build karma 265 completed (commit 1c0fabc3b1 by @falsandtru)

devoto13 added a commit to devoto13/karma that referenced this pull request May 14, 2020
devoto13 added a commit to devoto13/karma that referenced this pull request May 14, 2020
devoto13 added a commit to devoto13/karma that referenced this pull request May 15, 2020
devoto13 added a commit to devoto13/karma that referenced this pull request 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 issue has been resolved in version 5.0.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@falsandtru falsandtru deleted the patch-1 branch May 16, 2020 18:04
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.

5 participants