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

feat: upgrade to node 12 #451

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

feat: upgrade to node 12 #451

wants to merge 2 commits into from

Conversation

radiantspace
Copy link

@radiantspace radiantspace commented Jan 27, 2021

  • 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
@@ -48,7 +48,11 @@ if (typeof process.browser === 'undefined') {

// cleanup generated rules in the highlighter registry if they are idle for 2000ms
// 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,

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?

Copy link
Author

@radiantspace radiantspace Jan 29, 2021

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.

@@ -4,7 +4,7 @@ notifications:
email: false

node_js:
- 8
- 12

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