Reduced risk of false positives #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e html regex stricter, adding many more filetypes to the default exclusion list, and adjusting the html/rules tests. Fixes BrowserSync/browser-sync#198
|
Now THIS is why I love Open Source - great work! This looks flawless to me - I'll give it a good test in some real-world projects before we merge, but this is really great. It would be nice to get this into BrowserSync Many thanks :) |
shakyShane
added a commit
that referenced
this pull request
Jul 14, 2014
Reduced risk of false positives
|
I think you can really push this module further and make it the best-in-class. Would you like commit access? That way you can work on branches and I can swoop in to do the releases when needed? |
|
Sure, I'd be happy to. I'll probably wait to make any changes until I there is some feedback. |
|
Of course :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reduced risk of false positives for inserting the script by making the html regex stricter, adding many more filetypes to the default exclusion list, and adjusting the html/rules tests. Fixes BrowserSync/browser-sync#198
These are the changes I've made to help reduce the risk of accidentally corrupting a non-html file:
Increased the list of ignore types significantly. The new list should cover just about any form of file you'd expect to see served, and then some. It may be too aggressive, feel free to have me remove some if needed.
Increased the robustness of the html-test regex. The new regex requires an html-like tag to be the first non-whitespace character. It also allows for byte order marks, although I'm not sure if that's necessary.
I changed the order of the tests for injection. The original test went something like:
The problem is that non-html files might contain the rule, and end up being modified, which doesn't make much sense. The new logic is this:
This ensures that only content that looks like HTML will get modified.
I also rewrote the binary tests to allow for easily adding static tests of various files. I added a few new files to test, including a random binary blob that includes
<html>in the middle, and several JSON files with html content, to make sure they don't get modified.I ran the new tests with and without the new HTML regex and testing order, to ensure they are able to correctly test the results.
All other tests were left unmodified, and everything is passing.