Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch htmlparser2 instead of rely on MarkBind fork #948

Merged
merged 6 commits into from
Dec 8, 2019

Conversation

acjh
Copy link
Contributor

@acjh acjh commented Dec 7, 2019

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [ ] Enhancement to an existing feature
• [x] Other, please explain:

Patch htmlparser2

Resolves #396
Resolves #582
Closes #606
Unblocks #819

What is the rationale for this request?

  • We have not found a way to force Cheerio 0.22.0 to use MarkBind's htmlparser2 reliably.
  • We're on npm 5.8.0 mainly because of the earlier point (and a lesser issue of package-lock.json).
    Not being able to use the latest version of npm seems rather backward for users and devs.

What changes did you make? (Give an overview)

  1. Update package-lock.json using npm 6.13.2 (current latest; Node v12.13.1) — breaks test
  2. Limit js-beautify from ^1.6.12 to ~1.7.5 to have no output diff — fixes test broken by 1
  3. Update htmlparser2 version to 3.10.1 — breaks test
  4. Patch htmlparser2 based on Update Tokenizer to treat Markdown code as text instead of HTML htmlparser2#1 — fixes test broken by 3

Provide some example code that this change will affect:

-

Is there anything you'd like reviewers to focus on?

Is this the lesser evil? At least, this should have reliable behaviour for end-users regardless of their npm version.

Testing instructions:

  • Check that npm i using npm 6.12.1 (packaged with Node.js LTS version 12.13.1) or higher doesn't modify package-lock.json.
  • Check the output diff on an existing project that uses MarkBind.
    1. Clone the repo (e.g. https://github.com/nus-cs2103/website-base).
    2. Install MarkBind v2.6.0 (or checkout master branch + run npm ci).
    3. Negate (or comment out) _site/ in .gitignore:
      - _site/
      + !_site/
    4. Modify Site.prototype._setTimestampVariable in MarkBind source code for cleaner diff later.
      - this.userDefinedVariablesMap[base].timestamp = time;
      + this.userDefinedVariablesMap[base].timestamp = 'Sat, 07 Dec 2019 08:00:00 GMT';
    5. Run markbind build.
    6. Stage (or commit) all the changes, including _site directory.
    7. Install (or checkout + run npm ci) this PR.
    8. Modify Site.prototype._setTimestampVariable again if you need to.
    9. Run markbind build.
    10. The only diff should be the log file (e.g. _site/_markbind/logs/markbind-2019-12-07.log).

Let's patch Tokenizer to treat Markdown code as text instead of HTML.

From MarkBind/htmlparser2#1:

> This fix allows Markdown code to contain '<' , '<=' without having it
> affect other HTML elements as it is now treated as a text element.
> Furthermore, no spaces are required when typing these symbols within
> the back ticks.
>
> As such, inequalities like the above can be rendered normally as
> shown below.
>
> `x<y`
> `<`
> `<=`
> `x<=y`
Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

While it is true that this fix would be less elegant for the devs comparing to using the fork, I think it is acceptable for now as the changes needed for the patching is pretty minor. I think it will also save us some work in the long run, since upgrading to a newer version of htmlparser2 no longer involves having to fiddle around with our fork and resolve merge conflicts, but just merely finding out how to modify the patch.

I tested it accordingly to your instructions and it looks good to me 👍.

@acjh acjh changed the title [WIP] Patch htmlparser2 instead of rely on MarkBind fork Patch htmlparser2 instead of rely on MarkBind fork Dec 8, 2019
@yamgent yamgent merged commit aed7619 into MarkBind:master Dec 8, 2019
@yamgent yamgent added this to the v2.6.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Force cheerio to use our own custom htmlparser2 User guide: improve installation instructions for Linux
2 participants