Skip to content
This repository was archived by the owner on Apr 3, 2024. It is now read-only.

feat: upgrade to node 12#451

Draft
radiantspace wants to merge 2 commits into
masterfrom
node-12
Draft

feat: upgrade to node 12#451
radiantspace wants to merge 2 commits into
masterfrom
node-12

Conversation

@radiantspace

@radiantspace radiantspace commented Jan 27, 2021

Copy link
Copy Markdown
  • Update highlights to use newer oniguruma dependency, which otherwise won't compile on Node 12.
  • Remove cleanup grammar code, which doesn't work with newer highlights. While trying to solve some memory consumption issues, the code is actually an antipattern, which mutates the state of highlighter object. I kept the code for now to discuss how we want to proceed with the change.

Closes #442

Related to github/npm#1814

@radiantspace radiantspace self-assigned this Jan 27, 2021
@radiantspace radiantspace requested a review from a team January 28, 2021 12:46
Comment thread lib/render.js
// they take a tremendous amount of memory if you process many languages in a server type environment.
cleanup(highlighter.registry.grammars)

// this doesn't work with new highlights version,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is 3.1.4 actually newer than 3.2.0-candidate.1? I think it's not.
Is there any problem keeping the candidate one and keep cleanup code then?

@radiantspace radiantspace Jan 29, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Actually 3.1.4 is newer:
Screen Shot 2021-01-29 at 12 46 54

The reason for changing to newer version of highlights is to update oniguruma dependency, which is the actual blocker for supporting Node 12.

Comment thread .travis.yml

node_js:
- 8
- 12

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd also add .nvmrc since we're touching this repo.

@radiantspace radiantspace marked this pull request as draft January 29, 2021 10:54
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.

Support newer versions of Node

3 participants