Improve file type detection

[falsandtru]

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]

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

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[falsandtru]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[AppVeyorBot]

✅ Build karma 2648 completed (commit a12d56d15d by @falsandtru)

[karmarunnerbot]

✅ Build karma 250 completed (commit a12d56d15d by @falsandtru)

[karmarunnerbot]

✅ Build karma 249 completed (commit a12d56d15d by @falsandtru)

[falsandtru]

Looks like a validation error of cla.

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

[devoto13]

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]

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

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

[AppVeyorBot]

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

[karmarunnerbot]

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

[karmarunnerbot]

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

[karmarunnerbot]

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

[karmarunnerbot]

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

[AppVeyorBot]

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

[falsandtru]

Can you merge?

[devoto13]

@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]

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

[falsandtru]

done

[karmarunnerbot]

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

[AppVeyorBot]

✅ Build karma 2661 completed (commit 70989c2f70 by @falsandtru)

[devoto13]

Is there any specific reason why we need || '' here?
Otherwise LGTM.

[devoto13]

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

[falsandtru]

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.

[falsandtru]

Hm, looks like the source code treats paths only as URLs.

[devoto13]

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

[falsandtru]

Not sure. You should do such refactoring yourself.

[devoto13]

Okey, I'll try and submit an alternative PR in the coming days.

[devoto13]

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]

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]

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

[AppVeyorBot]

✅ Build karma 2662 completed (commit 7f96c568ed by @falsandtru)

[karmarunnerbot]

✅ Build karma 263 completed (commit 215a9ed33e by @falsandtru)

[AppVeyorBot]

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

[karmarunnerbot]

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

[karmarunnerbot]

✅ Build karma 264 completed (commit c5d5e7d4c8 by @falsandtru)

[AppVeyorBot]

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

[karmarunnerbot]

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

[karmarunnerbot]

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

[karmarunnerbot]

🎉 This issue has been resolved in version 5.0.7 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀