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

chore(deps): adapt to marked@12 #280

Merged
merged 3 commits into from Apr 30, 2024
Merged

chore(deps): adapt to marked@12 #280

merged 3 commits into from Apr 30, 2024

Conversation

stevenjoezhang
Copy link
Member

@stevenjoezhang stevenjoezhang commented Apr 3, 2024

Marked has undergone significant upgrades in the past year (the version number quickly jumped from 4 to 12), and many old customization methods no longer apply. This pull request is intended to adapt to the new versions of marked. It's important to note that I have only ensured that all current unit tests can pass; whether it works properly in a real Hexo environment still needs further testing.

Issue resolved: #209
Issue resolved: #263

@coveralls
Copy link

coveralls commented Apr 3, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling 4d07ec6 on marked
into 680f3ae on master.

@tomap
Copy link
Contributor

tomap commented Apr 3, 2024

Many options were removed in version 8.
https://marked.js.org/using_advanced#options
The doc should probably be re adapted.

Also we have our own implementation of headerid, while there is a plugin for that. We could remove our code for that

@stevenjoezhang
Copy link
Member Author

hexo-renderer-marked has custom implementation of many options, so it's not affected much by the breaking changes since marked 8.0. I'll check again if some options are no longer working.

@stevenjoezhang
Copy link
Member Author

stevenjoezhang commented Apr 4, 2024

marked's official headerid plugin does not support headerlink like <a href="#example" class="headerlink" title="Example"></a>, we may still need to use our own implementation

Ours:

return `<h${level} id="${id}"><a href="#${id}" class="headerlink" title="${stripHTML(text)}"></a>${text}</h${level}>`;

Theirs:
https://github.com/markedjs/marked-gfm-heading-id/blob/e214c9091ce5770b416ebede1f5076cc8026d5d7/lib/index.cjs#L107

Copy link
Member

@yoshinorin yoshinorin left a comment

Choose a reason for hiding this comment

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

marked requires node+18 since v10.0.0 😩

We have three options:

  1. Bump the marked to 9.x (Do not bump 10.x+)
  2. Require Node.js 18.x+ (all of hexo org repositories).
  3. Ignore it (fortunately, tests are passing)

Personally, I think we should start to drop Node.js 14 & 16. However, we have a lot of repositories... 😫

super();
this._headingId = {};
this.hexo = hexo;
function mangleEmail(text) {
Copy link
Member

Choose a reason for hiding this comment

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

LGTM, but why not use the plugin?
https://www.npmjs.com/package/marked-mangle

Copy link
Member Author

Choose a reason for hiding this comment

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

Using the official plugin is also an option, but I think using our custom implementation would be more convenient. The official marked plugin is implemented inside the tokenizer, while we have already customized links in the renderer (for example, modifying links that start with javascript:). This way, we can simply add a handling logic for mailto: links in a similar place.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we could re-implement all Hexo specific feature using marked extension API.

We could even implement the old filter API on top of the marked extension API.

@stevenjoezhang
Copy link
Member Author

Node.js 16.x is End-of-Life since September 2023, we can consider raising the minimum supported Node.js version for Hexo-related repositories.

@uiolee
Copy link
Member

uiolee commented Apr 6, 2024

Personally, I think we should start to drop Node.js 14 & 16. However, we have a lot of repositories... 😫

I think if there are no changes of code or dependencies , then there is no need to update the version numbers of all repositories.

@yoshinorin
Copy link
Member

yoshinorin commented Apr 8, 2024

then there is no need to update the version numbers of all repositories.

I'm concerned about the tedious PRs of just rewriting 'engines' and 'CI settings' in many repositories.

It may demotivate us (maintainers). I think this will be resolved in the migration to monorepo of hexojs/hexo#5273 in the future, but I don't think we can do that before the drop of Node.js 14 and 16.

Anyway, I think it's okay to start to drop Node.js 14 and 16.

@tomap
Copy link
Contributor

tomap commented Apr 8, 2024

CI is green, so adding node 20 and dropping node 14 and 16 can be done later

@stevenjoezhang
Copy link
Member Author

stevenjoezhang commented Apr 9, 2024

We might consider whether we can automate the process of upgrading the Node.js version used in actions/setup-node, for example, always testing with the latest three Node.js LTS versions.

For example, we can use tools like https://github.com/msimerson/node-lts-versions

@stevenjoezhang
Copy link
Member Author

stevenjoezhang commented Apr 9, 2024

By looking at the https://node.green website, I found that the recent Node.js versions haven't introduced many new features that are widely used. Therefore, even if marked raises its minimum supported version, it would still work fine on older versions like Node.js 14.

Update: The newer version of JSDOM is incompatible with Node.js 14, as it uses an operator that is not supported.

Copy link
Member

@yoshinorin yoshinorin left a comment

Choose a reason for hiding this comment

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

Let's move forward

@stevenjoezhang stevenjoezhang merged commit 62b9733 into master Apr 30, 2024
46 checks passed
@stevenjoezhang stevenjoezhang deleted the marked branch April 30, 2024 09:33
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.

Bold rendering issues Use marked.js official heading id implementation
6 participants