Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

fix bug: matching scripts in htmlmined html file #620

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix bug: matching scripts in htmlmined html file #620

wants to merge 1 commit into from

Conversation

ytxiuxiu
Copy link

@ytxiuxiu ytxiuxiu commented Apr 7, 2016

The original regex for matching <script src="..." in html file can only match script tags in different lines.
If the content of a html file (for example a html file which has been htmlmined) looks like this:

<script src="js/vendor-437841c1.js"></script><script src="js/script-9d21274e.js"></script>

It can not be matched correctly.
New regex could solve this problem.

The original regex for matching <script src="..." in html file can only match script tags in different lines.
If the content of a html file (for example a html file which has been htmlmined) looks like this:

<script src="js/vendor-437841c1.js"></script><script src="js/script-9d21274e.js"></script>

It can not be matched correctly.
New regex could solve this problem.
@eddiemonge
Copy link
Member

so it needs a non-greedy lookup? I suppose this is one way of doing it. Once actual html parsing is in then this shouldn't be needed.

@@ -7,7 +7,7 @@ var chalk = require('chalk');
var _defaultPatterns = {
html: [
[
/<script.+src=['"]([^"']+)["']/gm,
/<script[^\>]+src=['"]([^"']+)["']/gm,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could possibly also be:

<script[^>]+?src=['"]([^"']+)["']

@ytxiuxiu
Copy link
Author

ytxiuxiu commented Apr 7, 2016

@eddiemonge
Sorry, I didn't get why this shouldn't be needed once actual html parsing is in?

@eddiemonge
Copy link
Member

Once we can parse the html then most of the regexes won't be required since we will be able to do something like document.script.src

@ytxiuxiu
Copy link
Author

ytxiuxiu commented Apr 7, 2016

@eddiemonge
Cool! Get it, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants