Plugin opts filter #295

User4martin · 2017-08-16T05:55:25Z

This branch contains my entire work.

It is ready for review. Please see #288 for an overview what changed.

mde · 2017-08-31T21:15:30Z

lib/ejs.js

-    case '_' + d + '>':
-      if (this.mode == Template.modes.LITERAL) {
-        this._addOutput(line);
+    var isHandled;


Not a huge deal here, but I generally prefer to hoist all the declarations to the very top, to reflect what the runtime actually does.

mde · 2017-08-31T21:21:18Z

lib/ejs.js

-        case Template.modes.COMMENT:
-              // Do nothing
+      if (isHandled && this.mode != Template.modes.LITERAL) {
+        if (this.isInTag)


Minor quibble: isInTag implies a Boolean value. I can see you're leaning on the truthy/falsy in JS here, and letting this property do double-duty by holding an actual value -- but ideally if the property name looks Boolean (e.g., isSomething, hasSomething, etc.) it should actually be a Bool.

mde · 2017-08-31T21:22:31Z

lib/ejs.js

-        // In string mode, just add the output
-      else {
-        this._addOutput(line);
+      if (! isHandled) {


Style issue: space between ! "not" operator and variable name is not needed. Should be simply !isHandled.

mde · 2017-08-31T21:24:31Z

lib/ejs.js

+                  includeOpts.filename));
+              return; // TODO: debug info
+            }
+            // END HACH include


Spelling: "HACK," not "HACH."

mde · 2017-08-31T21:25:25Z

lib/ejs.js

        self.scanLine(line);
      });
-      if (this.isInTag)
+      if (this.isInTag) {


In my experience, omitted brackets are a major source of confusion during refactoring.

mde · 2017-08-31T21:25:36Z

lib/ejs.js

@@ -701,8 +702,9 @@ Template.prototype = {
        isHandled = false;
      }
      if (isHandled && this.mode != Template.modes.LITERAL) {
-        if (this.isInTag)
+        if (this.isInTag) {


mde · 2017-08-31T21:29:44Z

lib/ejs.js

+  this.regexLiteral = this.createRegex(_REGEX_STRING_LITERAL, 'g');
+  this.regexLiteralReplace = _REGEX_STRING_LITERAL_REPL;
+
+  this.modeMap = {


Awesome! I have wanted to organized the modes like this for a long time!

mde · 2017-08-31T22:37:41Z

lib/ejs.js

@@ -478,7 +503,18 @@ function Template(text, opts) {

  this.opts = options;

-  this.regex = this.createRegex();
+  this.regexOpen = this.createRegex(_REGEX_STRING_OPEN);


Let's keep this stuff internal.

mde · 2017-08-31T22:42:28Z

lib/ejs.js

+    switch (this.mode) {
+    case Template.modes.EVAL:
+    case Template.modes.RAW:
+      // HACK: backward-compat `include` preprocessor directives


Maybe it's worth a major version bump just to jettison this silly preprocessor stuff.

mde · 2017-08-31T22:50:59Z

lib/ejs.js

+        includeOpts = utils.shallowCopy({}, self.opts);
+        includeObj = includeSource(include[1], includeOpts);
+        if (self.opts.compileDebug) {
+          includeSrc =


Might be worth moving all this inline stuff to a utility function.

mde · 2017-08-31T22:59:07Z

lib/ejs.js

-      this.isAfterTag = false;
+    }
+
+    switch (this.mode) {


Unless we're actively using fall-through, might be worth considering doing some sort of dispatch via properties / named methods on an object, e.g.:

this.processors = {}; this.processors[Template.modes.EVAL] = function (line) { this.source += ' ; ' + line + '\n'; }; // Etc.... var processor = this.processors[this.mode]; if (typeof processor == function) { processor.call(this, line); }

Something like that. If the function doesn't exist, throw some sort of reasonable error.

User4martin · 2017-09-19T19:41:27Z

If I am not mistaken most of your reviews were on individual commits. Most of the code you commented on is not present in the final commit.

I did look for variables that where declared not on top of their block, and moved (most) of them up.
Though

if (cond) {
    var foo;

still is not the same as on top of the function. But jslint would complain if used outside the block, so it should be safe.

About the old inline file (none function style):
If you want to get rid of it, I can move it to a plugin. Then it will no longer be in the main lib/ejs.js file.
If you want to keep it, I don't think it needs to move, but (if you wish) I can move some of the string constants to the function "includeSource".

User4martin · 2017-10-22T22:55:43Z

any updates?

RyanZim · 2017-10-23T20:25:50Z

@User4martin Sorry for being so slow to respond here.

To be honest, I do not have the time to review this huge diff. This basically amounts to a nearly total rewrite of EJS. I cannot review all that code and be confident that there aren't regressions. I can't review commit-by-commit, because we have commits like linting which are pure noise.

I'd prefer if this were split into several PRs, with clearly defined commits for each particular change, with no noise commits to fix typos, linting, comments, etc.

Sorry if this comes across as blunt; I realize you invested quite a bit of time into this PR; but the last time I checked, EJS was in the top 100 packages on npm. At that rank, we cannot afford to have regressions, even for edge cases. And I do not have an unlimited amount of time to review code here; especially since I don't make any money doing this. All the same, my apologies for letting this sit so long without a proper response.

User4martin · 2017-10-23T21:06:51Z

I understand the time issue. This is why I waited a while before asking.

There are "smaller" changesets available. A breakdown into 4 or 5 sets. I made a branch for each of them and then based the next branch on this.

I though initially that small steps would be easier, but somewhere along the way was mislead to think you like one big pull request instead.

However:
Each of those steps should pass all tests (if not, that I will fix).
Not each of them will have all your other coding style requirements (such as var declared on top of block)

Fixing all the variables for example makes limited sense, if the next changeset will remove the variable entirely.

I created pull request #311.

This is the first complete change.

See issue #288 for an overview of all the change-sets (well on top of 288 there are then the addition of the new plugins.)

If #311 is within review-able limits, then we can take it from there. (I may need some time to do any changes, if required)

If you want I can already create pull request for all the other changes (But they need to be done in order, as they base on each other)

I suggest, if you review it, you create a new branch, so coding style issues can be ignored, and addressed when all pull request are in this branch.

Of course, if it can be merged to the main branch, it is a complete change in itself.
And then later pull requests can still go into a branch.

… / throw error on unmatched/unopened closing tags

User4martin mentioned this pull request Aug 16, 2017

Test tags #277

Closed

mde reviewed Aug 31, 2017

View reviewed changes

lib/ejs.js Outdated

includeOpts.filename));

return; // TODO: debug info

}

// END HACH include

Copy link

Owner

mde Aug 31, 2017

Choose a reason for hiding this comment

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

Spelling: "HACK," not "HACH."

mde reviewed Aug 31, 2017

View reviewed changes

User4martin mentioned this pull request Oct 23, 2017

Refactor generate source #311

Closed

m added 13 commits October 23, 2017 22:36

move closing tag check to scanLine

3f39a80

move old include code to scanLine

7250566

refactor parser, one full tag per scanline

306f962

remove mode LITERAL / no longer needed

0d01573

changed literal tags to be standalone (no longer affect the next tag)…

8bc8316

… / throw error on unmatched/unopened closing tags

speed up, run regex for literals only if needed

fdb09af

preview next-open/prev-close tag

01703cd

fix slurping for <%_ _%> with other delim

f48ebaa

refactor [-_]%> new line slurp

e14c2e7

combine __append calls

1877669

only calculate newLineCount when compileDebug = true

bd966be

sample code for mixin

95d88f4

improve sample code / rename to snippet

9a94c4f

m added 22 commits October 23, 2017 23:43

tests / fix

419a284

adapt to changes in ejs

8980ec2

allow new modes added by plugin

f98a6ec

moved modes to exports for easier access in plugins

b23a184

cache argument names

8878c38

linting

cecf459

moved snippet to plugin folder

d0fdc47

improve tests / run with/without plugin

4d228bc

fix: do not create args when opts.client is set

089723d

docs

4837daa

rename key/method names to comply with docs

fd0b34a

improve build args

ddc1c7e

fixing docs markup

65e5138

amend docs

9aaa9ca

combine open/close pattern

565354c

add a hook for fs.existsSync, same as already exists for readFileSync

4fae4d9

added cache for file exists

772c81e

plugin locals

72921a2

opts-filter

cd0740f

copy opts, so data/locals is not modified

68d8eb6

move "var" declarations towards top of blocks

2953a63

remove comment

05df465

User4martin force-pushed the plugin-opts-filter branch from 9c80970 to 05df465 Compare October 23, 2017 22:51

User4martin mentioned this pull request Dec 20, 2017

"Require" Pattern for Actual Reusable Dynamic Functions Alongside Traditional Includes #328

Closed

mde deleted the branch mde:master May 30, 2022 16:34

mde closed this May 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin opts filter #295

Plugin opts filter #295

User4martin commented Aug 16, 2017

mde Aug 31, 2017

mde Aug 31, 2017

mde Aug 31, 2017 •

edited

Loading

mde Aug 31, 2017

mde Aug 31, 2017

mde Aug 31, 2017

mde Aug 31, 2017

mde Aug 31, 2017

mde Aug 31, 2017

mde Aug 31, 2017

mde Aug 31, 2017

mde Aug 31, 2017 •

edited

Loading

User4martin commented Sep 19, 2017

User4martin commented Oct 22, 2017

RyanZim commented Oct 23, 2017 •

edited

Loading

User4martin commented Oct 23, 2017 •

edited

Loading

Plugin opts filter #295

Plugin opts filter #295

Conversation

User4martin commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mde Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mde Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

User4martin commented Sep 19, 2017

User4martin commented Oct 22, 2017

RyanZim commented Oct 23, 2017 • edited Loading

User4martin commented Oct 23, 2017 • edited Loading

mde Aug 31, 2017 •

edited

Loading

mde Aug 31, 2017 •

edited

Loading

RyanZim commented Oct 23, 2017 •

edited

Loading

User4martin commented Oct 23, 2017 •

edited

Loading