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

feat: html anchor support #5

Merged
merged 9 commits into from
Jan 12, 2024
Merged

Conversation

igor-tsiglyar
Copy link

@igor-tsiglyar igor-tsiglyar commented Jan 11, 2024

What changes this PR introduce?

Plugin now recognizes links to anchors, either by id or by name. I tried to stick to the way markdownlint does that, so borrowed a helper to extract the anchor attribute from there.

List any relevant issue numbers

Addresses #4

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

Had some issues with passing the commit checks. I am not very familiar with JS ecosystem, so resolved them the way I could. Feel free to propose a better solution.

  1. Had various problems reported by tsc like:
> [email protected] lint:javascript
> tsc --project jsconfig.json --noEmit
node_modules/markdownlint-rule-helpers/helpers.js:121:31 - error TS2345: Argument of type 'Object' is not assignable to parameter of type 'string'.
121   return isUrl(url) ? new URL(url) : url; 

so excluded node_modules in jsconfig.json

  1. Had a module error while running the tests:
> node --test --experimental-test-coverage ./test

node:internal/modules/cjs/loader:1147
  throw err;
  ^

Error: Cannot find module '/home/dev/repos/markdownlint-rule-relative-links/test'                                                         at Module._resolveFilename (node:internal/modules/cjs/loader:1144:15)                                                                 at Module._load (node:internal/modules/cjs/loader:985:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:135:12)
    at node:internal/main/run_main_module:28:49 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Node.js v21.2.0
✖ test (39.481616ms)
  'test failed'

Removing ./test from node command fixed that, maybe that is due to my node version is newer?

  1. Had declaration error reported by tsc:
> tsc --project jsconfig.json --noEmit

src/utils.js:2:40 - error TS7016: Could not find a declaration file for module 'markdownlint-rule-helpers'. '/home/dev/repos/markdownlint-rule-relative-links/node_modules/markdownlint-rule-helpers/helpers.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/markdownlint-rule-helpers` if it exists or add a new declaration (.d.ts) file containing `declare module 'markdownlint-rule-helpers';`

2 const { getHtmlAttributeRe } = require("markdownlint-rule-helpers")

Perhaps this is because this function has only been recently introduced to markdownlint-rule-helpers and the corresponding @types has not been yet updated. I figured since there is no TS used anyway I would fix this with @ts-ignore.

@theoludwig theoludwig self-requested a review January 11, 2024 19:15
@theoludwig theoludwig added the enhancement New feature or request label Jan 11, 2024
@theoludwig theoludwig linked an issue Jan 11, 2024 that may be closed by this pull request
@theoludwig
Copy link
Owner

theoludwig commented Jan 12, 2024

Thank you for your PR, it is highly appreciated! 😄

Note about markdownlint-rule-helpers

It's better to not include markdownlint-rule-helpers dependency, as stated in their README:

Undocumented - This package exports the internal functions as-is. The APIs were not originally meant to be public, are not officially supported, and may change from release to release.

And TypeScript types definitions is not included, giving you this tsc error: Could not find a declaration file for module 'markdownlint-rule-helpers'.

It's better to directly include the functions we need in our source code.
We only use 2 functions anyway. If somehow, we need to use more stuff from this library, and we notice that the functions we use are frequently updated, we might reconsider this, but for the time being it should be fine.

Node.js Test Runner

Removing ./test from node command fixed that, maybe that is due to my node version is newer?

Yes, I'm using Node.js LTS Release lines, currently v20.11.0. I tried installing v21.5.0 to check the issue, and indeed, it is broken. I think this might be related to this semver-major commit: nodejs/node#47653

Removing ./test is fine, as it also works with Node.js v20 and is even shorter/simpler, so yeah good change. 👍

Implementation

Your implementation had a few bugs (e.g: not supporting <div id="fragment"></div>, only supporting anchors, the check of the fragments was also occuring in its own file, which MD051 rule already validate, so the errors would be duplicated/twice, and others bugs).

I've fixed all those bugs and added many test cases. 👍
The feature is ready to be released!

@theoludwig theoludwig merged commit 146f904 into theoludwig:develop Jan 12, 2024
2 checks passed
Copy link

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@igor-tsiglyar
Copy link
Author

@theoludwig Thanks, awesome collaboration!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add html anchor support
2 participants