feat(index): add support for `{RegExp}` directives (`options.directives`) #27

oantoro · 2018-01-18T12:56:45Z

Example:
The custom directive {name: /\?(php|=).*/, start: '<', end: '>'} will match any directive with <?php ?> or <?= ?> pattern.

Note:
I thought that this commit will solve #26 (comment) if we use regular expression directive started with <% but after I checked the library, I found that any tag started with % for example <%=name%> will be treated as a tag by the library, so this tag will never touch onprocessinginstruction event. So this is an exception.

Example: The custom directive {name: /\?(php|=).*/, start: '<', end: '>'} will match any directive with <?php ?> or <?= ?> pattern

coveralls · 2018-01-18T13:00:05Z

Coverage remained the same at 100.0% when pulling 9403660 on okyantoro:regex-directive into 923b16a on posthtml:master.

Scrum · 2018-01-18T14:26:09Z

index.js

+                isDirective = true;
+            } else if (tagName === directive.name) {
+                isDirective = true;
+            }


one condition is better:

if ((directive.name instanceof RegExp) && directive.name.test(tagName)) { isDirective = true; } if (tagName === directive.name) { isDirective = true; }

coveralls · 2018-01-18T14:37:13Z

Coverage remained the same at 100.0% when pulling 1b800f7 on okyantoro:regex-directive into 923b16a on posthtml:master.

coveralls · 2018-01-18T14:58:53Z

Coverage remained the same at 100.0% when pulling 2ca0e4e on okyantoro:regex-directive into 29944dc on posthtml:master.

michael-ciniawsky · 2018-01-18T16:53:54Z

index.js

        for (var i = 0; i < directives.length; i++) {
            var directive = directives[i];
            var directiveText = directive.start + data + directive.end;
+            var isDirective = false;


function isDirective (directive, tag) { if (directive.name instanceof RegExp) { const regex = RegExp(directive.name, 'i') return regex.test(tag) } if (tag !== directive.name) { return false } return true }

Untested, but please a make this a private helper function instead of n variables and {Boolean} switches/assignments

this line const regex = RegExp(directive.name, 'i')
const is not supported by es5 and RegExp(directive.name, 'i') only works in ES6.
Related to MDN

Starting with ECMAScript 6, new RegExp(/ab+c/, 'i') no longer throws a TypeError ("can't supply flags when constructing one RegExp from another") when the first argument is a RegExp and the second flags argument is present. A new RegExp from the arguments is created instead.

So we change the const to var and RegExp(directive.name, 'i') to RegExp(directive.name.source, 'i')
Thank you

Yep, const sadly needs to var here 😞

var pattern = /a/ var regex = RegExp(pattern, 'i') console.log(regex.test('Abc'))

⚠️ Note that it's RegExp(pattern, flags) not new RegExp(string, flags) (No new)

michael-ciniawsky · 2018-01-18T16:54:23Z

test/test.js

    });

+    it('should be parse regular expression directive', function() {
+        var customDirectives = {directives: [


customDirectives => options && \n after {

michael-ciniawsky · 2018-01-18T16:55:28Z

test/test.js


+    it('should be parse regular expression directive', function() {
+        var customDirectives = {directives: [
+            {name: /\?(php|=).*/, start: '<', end: '>'}


\s after { && } please

coveralls · 2018-01-19T08:53:09Z

Coverage remained the same at 100.0% when pulling 0da0c81 on okyantoro:regex-directive into 04200e6 on posthtml:master.

michael-ciniawsky · 2018-01-19T12:37:43Z

index.js

            var directiveText = directive.start + data + directive.end;
-            var isDirective = false;

            tagName = name.toLowerCase();


Just reassign name here

coveralls · 2018-01-19T13:58:18Z

Coverage remained the same at 100.0% when pulling 6f046c8 on okyantoro:regex-directive into 04200e6 on posthtml:master.

michael-ciniawsky

@okyantoro Thx

michael-ciniawsky · 2018-01-19T14:14:06Z

index.js


+    function isDirective(directive, tag) {
+        if (directive.name instanceof RegExp) {
+            var regex = RegExp(directive.name.source, 'i');


Final nitpick directive.name.source => directive.name (But both work)

travis gives this error if we give regex parameter for regex and supply flag.

1) PostHTML-Parser test should be parse regular expression directive: TypeError: Cannot supply flags when constructing one RegExp from another at new RegExp (native) at RegExp (native) at isDirective (index.js:9:991) at Object.parserDirective [as onprocessinginstruction] (index.js:9:2022) at Parser.onprocessinginstruction (node_modules/htmlparser2/lib/Parser.js:275:13) at Tokenizer._stateInProcessingInstruction (node_modules/htmlparser2/lib/Tokenizer.js:345:13) at Tokenizer._parse (node_modules/htmlparser2/lib/Tokenizer.js:686:9) at Tokenizer.write (node_modules/htmlparser2/lib/Tokenizer.js:632:7) at Parser.write (node_modules/htmlparser2/lib/Parser.js:334:18) at postHTMLParser (index.js:9:5732) at parser (index.js:9:6155) at parserWrapper (index.js:9:6665) at Context.<anonymous> (test/test.js:143:16)

I have tried in f5249b2 and the test result https://travis-ci.org/posthtml/posthtml-parser/builds/330561437 is failed.

🙇 I misread the MDN snippet which clearly states starting from ECMAScript 2015 (6) it no longer throws. You're right for the current targets this definitely needs to be handled via pattern.source

Is there any plan to move to ES6?

we support the node> = 0.10

michael-ciniawsky · 2018-01-19T14:15:09Z

@gitscrum Is this really a semver major ? :) Looks like a minor (feature)...

Scrum · 2018-01-23T15:19:28Z

@okyantoro thanks for your contribution, we really appreciate this and are looking forward to a new pool of requisites 👍

oantoro · 2018-01-24T01:47:55Z

Thank you @gitscrum

directive: add support for regular expression custom directive.

9403660

Example: The custom directive {name: /\?(php|=).*/, start: '<', end: '>'} will match any directive with <?php ?> or <?= ?> pattern

Scrum requested review from voischev and Scrum January 18, 2018 14:20

Scrum modified the milestones: 0.3.2, 0.3.3 Jan 18, 2018

Scrum added pr: review status: pending semver: major ci: ok type: perf labels Jan 18, 2018

Scrum requested changes Jan 18, 2018

View reviewed changes

Scrum changed the title ~~directive: add support for regular expression custom directive.~~ [perf]: add support for regular expression custom directive. Jan 18, 2018

split directive checking per condition

1b800f7

Scrum modified the milestones: 0.3.3, 0.4.0 Jan 18, 2018

Scrum self-assigned this Jan 18, 2018

Merge branch 'master' into regex-directive

2ca0e4e

Scrum approved these changes Jan 18, 2018

View reviewed changes

michael-ciniawsky changed the title ~~[perf]: add support for regular expression custom directive.~~ feat: add support for {RegExp} directives (options.directives) Jan 18, 2018

michael-ciniawsky requested changes Jan 18, 2018

View reviewed changes

michael-ciniawsky added type: feature and removed type: perf labels Jan 18, 2018

michael-ciniawsky changed the title ~~feat: add support for {RegExp} directives (options.directives)~~ feat(index): add support for {RegExp} directives (options.directives) Jan 18, 2018

Scrum removed the request for review from voischev January 18, 2018 17:46

Scrum and others added 2 commits January 18, 2018 22:46

Merge branch 'master' into regex-directive

e90d08b

refactor directive parser

f7d296c

oantoro and others added 3 commits January 19, 2018 04:41

fix code style

01f8cad

build RegEx from RegEx.source

4bc2807

Merge branch 'master' into regex-directive

0da0c81

michael-ciniawsky reviewed Jan 19, 2018

View reviewed changes

lowercase and reassign 'name' var

6f046c8

michael-ciniawsky approved these changes Jan 19, 2018

View reviewed changes

Scrum added status: approved pr: merge semver: minor and removed pr: review status: pending semver: major labels Jan 19, 2018

Scrum merged commit 0752b3d into posthtml:master Jan 23, 2018

michael-ciniawsky removed ci: ok pr: merge labels Jan 23, 2018

oantoro deleted the regex-directive branch January 24, 2018 01:47

Uh oh!

feat(index): add support for {RegExp} directives (options.directives) #27

feat(index): add support for {RegExp} directives (options.directives) #27

Uh oh!

Conversation

oantoro commented Jan 18, 2018

Uh oh!

coveralls commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 18, 2018

Uh oh!

coveralls commented Jan 18, 2018

Uh oh!

michael-ciniawsky Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 19, 2018

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Scrum commented Jan 23, 2018

Uh oh!

oantoro commented Jan 24, 2018

Uh oh!

Uh oh!

feat(index): add support for `{RegExp}` directives (`options.directives`) #27

feat(index): add support for `{RegExp}` directives (`options.directives`) #27

michael-ciniawsky Jan 18, 2018 •

edited

Loading

michael-ciniawsky Jan 18, 2018 •

edited

Loading

michael-ciniawsky Jan 19, 2018 •

edited

Loading

michael-ciniawsky Jan 19, 2018 •

edited

Loading

michael-ciniawsky commented Jan 19, 2018 •

edited

Loading