🚧[Experimenting]: Refactoring marked js🚧 #1102

joshbruce · 2018-02-28T21:15:58Z

Marked version: 0.3.17

Description

Experimenting with the codebase to learn more and try to make marked easier to read without fundamentally breaking things.

Again, this is experimental, please do not merge!

Having said that, this should demonstrate how we can effectively alter how easy it is to read marked without actually changing the functionality. Please see code comments for questions I have regarding why certain decisions were made and whatnot.

I've also noted areas where I do not know if it is possible to test a unit of functionality with our current testing harness; therefore, tagging @UziTech.

joshbruce · 2018-02-28T21:27:20Z

I really don't trust our test bed to accurately reflect the desired behavior and use cases, to be honest.

After the lexer is run, the parser goes next, the InlineLexer is not used by Marked or the Lexer, only seems to be the Parser.

joshbruce · 2018-02-28T22:21:27Z

@intcreator: Interesting findings and notes in here, might help. Maybe we should slow down on trying to fix issues and work on making this thing more readable...?? Or, I'll do that, you do you, and we'll meet in the middle??

intcreator · 2018-03-01T05:12:41Z

It looks like the diff got messed up so it's really hard for me to tell what's going on still. As far as fixing vs. making the code legible, I vote we get the builds passing (just fixing #1092) then we can focus on understanding the code more. It's just way easier to check if things are still okay if builds are passing.

Edit: I see the builds are passing now so let's work on figuring out what the code does. This may be a good opportunity for creating some unit tests as well.

Edit 2: Okay, the diffs are messed up because you're reorganizing the layout of the code. Cool. I'm kind of liking the idea someone had earlier to have different things in different files. I think unit tests will make it easy to see what's going on more clearly too.

I'll look at it a bit when I have time and make comments where appropriate.

intcreator · 2018-03-01T06:19:48Z

lib/marked.js

+ * @return string
+ */
+function marked(src, opt, callback) {
+  // ^^ Can't we do something like marked(src, opt = {}, callback = function())


Default parameters are ES6 according to MDN. Maybe in 2.0.

ES5 is gonna probably start to annoy me soon. :)

Perhaps we can find a transpiler for 1.0.

intcreator · 2018-03-01T06:24:39Z

lib/marked.js

+    var highlight = opt.highlight,
+        tokens,
+        pending,
+        i = 0;


According to MDN, the comma operator evaluates each operand and returns the value of the last operand. Here, highlight = opt.highlight, tokens, pending, and i = 0 are all being evaluated and 0 is being returned (and not being used).

This is a somewhat obscure way of declaring tokens, pending, and i as variables in addition to highlight. Here is a reference on Stack Overflow.

Concur with the assessment of the commenter. Wonder if we can add that to the lint (@UziTech)? Don't do this. :)

I agree, I hate this style, but that is how chjj did it. I prefer initializing the variables as close to when you use them as possible.

In this case since the = operator has a higher presedence than the , operator the expression is essentially:

var (highlight = opt.highlight), (tokens), (pending), (i = 0);

To be fair, I don't know who necessarily did it that way, just that I think it should be changed. Totally agree that putting things as close to their contextual relevance is kind of a big deal.

There actually seems to be a lot of "clever" decisions that were made, which may not actually bring the desired ROI compared to writing human readable code. We spend way more time reading than we do writing, you know?

joshbruce · 2018-03-01T14:33:05Z

@intcreator: On the diff, don't know if this is what you're referring to exactly, but the moving marked() to the top causes the entire file to considered new according to the diff...basically a change to the whole file. I'm trying to make each commit a refactor; so, it might be easier to read one commit at a time.

intcreator · 2018-03-02T20:21:14Z

lib/marked.js

+    //
+    // TODO: Don't some highlighter plugins allow for `js`, effectively being less than three
+    //       despite wanting to have the highlighter run??
+    if (!highlight || highlight.length < 3) {


I think there are better tests than the length of the highlight. Agree with the possibility of js. Will we be supporting syntax highlighting of languages such as c or r? Highlighting doesn't appear to be in the CommonMark spec.

Agreed. See #1043

intcreator · 2018-03-02T20:23:58Z

lib/marked.js

+    }
+
+    return;
+  }


This function is almost 100 lines long. I highly recommend we break it down into pieces.

Tagging #1043 so UziTech can get a better feel on internal pull for the updates to the test harness request. No pressure. Just informational purposes.

Meant #1103

joshbruce · 2018-03-02T22:49:07Z

lib/marked.js

+ * @return boolean
+ */
+marked.receivedOnlySource = function(src, opt, callback) {
+  return (marked.receivedCallback(opt, callback));


Should be return !(marked.receivedCallback(opt, callback)); only source means we did not receive those other things.

joshbruce · 2018-03-02T22:57:41Z

different things in different files

Strictly speaking, is there anything about ES5 (or some other gotcha) that stops us from separating marked.js before 1.0 or later?

joshbruce · 2018-03-02T22:54:33Z

lib/marked.js

-  .getRegex();
+function Parser(options) {
+  this.tokens = [];
+  this.token = null;


Is having it be null necessary? Given this becomes an object with members might be easier for a new person to understand if they saw something like this (or make a Token object):

{ type: '', ... }

Was going to make a more complete example; however, it appears the Token is just a random set of members...like an undefined type. You can see it in the switch inside Parser.prototype.tok() ~ line 930

Maybe something more akin to this from traditional OO languages...using Swift highlighting, but not the syntax, strictly speaking.

abstract class Token {} class Heading extends Token {} // then the switch could be something like switch (typeof this.token.type) { case 'Heading': ...

But, if the type member is the only thing in common between all the different variations for Token...then there's really no reason for the type member or Token. ??

I wonder if there are more "hidden" objects inside of here.

joshbruce · 2018-03-03T00:08:31Z

lib/marked.js

+  } else if (this.options.pedantic) {
+    this.rules = inline.pedantic;
+  }
+}


The Renderer is just below here, it's essentially a string concatenation object...there's a separate one for both inline and block elements...think it could actually be much smaller.

When we get there, I'll probably want to work on this one...leverage the work from here and here. More the former than the latter since we need to accommodate client-side and most likely we are the extension of someone else's package, being used in, yet again, someone else's project.

We really should try to make Marked compile to something tiny - that's probably why some of the "clever" decisions were made, in fact. Not sure if the project started prior to the prevalence of minify for JS.

[edit] There's also the inability to minify the "boilerplate" mentioned by @UziTech in #746 - "prototype" can't get minified...and neither can any of the JS standard library calls and JS base language stuff.

joshbruce · 2018-03-03T00:38:30Z

It seems like what Marked is trying to do is sound, but it never got the chance to iterate that far...could be totally wrong here, but looking at the code, it's starting to feel familiar based on other projects:

M⬇ --- parse ---> {} --- render ---> HTML

As we consider the future refactoring and unit tests, might be worth keeping the following HTML element type definition in mind:

{
  // h1, h2, etc.
  elem: string,

  // the attribute list, which is usually a set of key-value pairs, JSON, in JS,
  // where you can easily convert keys to strings to generate the desired `key=value` 
  // string output. 
  attr: array, 

  // this gets tricky because some HTML elements can only take a string; or, there's the 
  // child element problem (what marked seems to use "inline" for). If we create a text 
  // object that only holds a string we can always pass in an array of element objects 
  // (what we seem to call tokens at the moment).
  inner: string|array,

  // this phrasing comes from the HTML5 spec when looking at the definition of an 
  // individual element, the first time I created this type definition I called 
  // `isSelfClosing` but that proved confusing.
  omitEndTag: boolean
}

This is almost the model used by React, I think, but not explicitly just through their createElement method: https://reactjs.org/docs/react-api.html#createelement

Maybe we need an Element object??

Then the parser can just keep creating those. Various Markdown flavors are also only able to use so many of the global attributes afforded by HTML5 that we might be able to do some optimization there as well.

joshbruce · 2018-04-05T18:50:27Z

Closing as edification was achieved, conflicts, and will most likely happen over a series of PRs not just one.

Josh Bruce added 6 commits February 28, 2018 15:10

Move marked to top

be82a73

Extract method of valid src check

0e00700

Rename valid src check

fb28a95

Notes

02200d7

Ah-ha!

8e1f2c4

Initial PR

0741381

joshbruce added RR - refactor & re-engineer Results in an improvement to developers using Marked, or end-users, or both. question labels Feb 28, 2018

Josh Bruce added 2 commits February 28, 2018 16:18

Caught by the linter, whoops.

f49c570

Update opt question

e1897c8

Josh Bruce added 9 commits February 28, 2018 16:28

Update marked() docblock

ad96e64

Move lexer to second position

db0f752

Move regex to second position, not defined yet errors - whoops

a4f97cc

Move Parser to third position

b988e85

After the lexer is run, the parser goes next, the InlineLexer is not used by Marked or the Lexer, only seems to be the Parser.

Leave InlineLexer in fourth position

1ea2bc8

Move all variables and functions to just after Marked

10384e5

Why are some inside the object and others aren't?

22f5f45

Notes on functions

9ce2430

Linting works. 😀

ca78f78

joshbruce mentioned this pull request Feb 28, 2018

[WIP]: Update contributing #1096

Closed

8 tasks

intcreator reviewed Mar 1, 2018

View reviewed changes

intcreator reviewed Mar 2, 2018

View reviewed changes

joshbruce commented Mar 2, 2018

View reviewed changes

joshbruce commented Mar 3, 2018

View reviewed changes

Feder1co5oave mentioned this pull request Mar 4, 2018

[linting] Use eslint to lint code. + minor refactoring + Travis CI setup #1020

Merged

This was referenced Mar 4, 2018

[Architecture]: Capture high-level as-is class diagram #1130

Closed

WIP: test: jasmine-ify original tests #1129

Closed

joshbruce closed this Apr 5, 2018

joshbruce deleted the refactoring-marked-js branch April 5, 2018 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚧[Experimenting]: Refactoring marked js🚧 #1102

🚧[Experimenting]: Refactoring marked js🚧 #1102

joshbruce commented Feb 28, 2018

joshbruce commented Feb 28, 2018

joshbruce commented Feb 28, 2018

intcreator commented Mar 1, 2018 •

edited

intcreator Mar 1, 2018

joshbruce Mar 1, 2018

intcreator Mar 2, 2018

intcreator Mar 1, 2018 •

edited

joshbruce Mar 1, 2018

UziTech Mar 1, 2018

joshbruce Mar 1, 2018

joshbruce Mar 1, 2018

joshbruce commented Mar 1, 2018

intcreator Mar 2, 2018

joshbruce Mar 2, 2018

intcreator Mar 2, 2018

joshbruce Mar 2, 2018

joshbruce Mar 2, 2018

joshbruce Mar 2, 2018

joshbruce commented Mar 2, 2018

joshbruce Mar 2, 2018 •

edited

joshbruce Mar 2, 2018 •

edited

joshbruce Mar 2, 2018

joshbruce Mar 3, 2018 •

edited

joshbruce commented Mar 3, 2018 •

edited

joshbruce commented Apr 5, 2018

+                  }
+                  return;
+                }

🚧[Experimenting]: Refactoring marked js🚧 #1102

🚧[Experimenting]: Refactoring marked js🚧 #1102

Conversation

joshbruce commented Feb 28, 2018

Description

joshbruce commented Feb 28, 2018

joshbruce commented Feb 28, 2018

intcreator commented Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intcreator Mar 1, 2018 • edited

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

joshbruce commented Mar 1, 2018

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

joshbruce commented Mar 2, 2018

joshbruce Mar 2, 2018 • edited

Choose a reason for hiding this comment

joshbruce Mar 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshbruce Mar 3, 2018 • edited

Choose a reason for hiding this comment

joshbruce commented Mar 3, 2018 • edited

joshbruce commented Apr 5, 2018

intcreator commented Mar 1, 2018 •

edited

intcreator Mar 1, 2018 •

edited

joshbruce Mar 2, 2018 •

edited

joshbruce Mar 2, 2018 •

edited

joshbruce Mar 3, 2018 •

edited

joshbruce commented Mar 3, 2018 •

edited