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

Add some plugins to list of plugins #1291

Merged
merged 4 commits into from
May 2, 2024
Merged

Conversation

rastislavcore
Copy link
Contributor

@rastislavcore rastislavcore commented Mar 1, 2024

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Added 3 plugins into documentation:

  • remark-fediverse-user
  • remark-corebc
  • remark-corepass

Added 3 plugins:
- remark-fediverse-user
- remark-corebc
- remark-corepass

Signed-off-by: Rastislav ₡ORE <[email protected]>
@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Mar 1, 2024

This comment has been minimized.

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a185821) to head (9216116).
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1291   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          142       142           
=========================================
  Hits           142       142           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Mar 1, 2024
Copy link
Member

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @rastislavcore!
Appreciate you sharing these.

A few thoughts which may benefit the projects:

@rastislavcore
Copy link
Contributor Author

@ChristianMurphy Good catches!

Mostly all of them integrated.

  1. I would like to keep Typescript syntax. Please, advise if that must be changed.
  2. ✅ (excluded require)
  3. I had some difficulties with node and jest as well. Uvu did the job, I will consider to replace later.
  4. Not for now, but if you can clarify more I will consider.

@ChristianMurphy
Copy link
Member

Thanks @rastislavcore!
Appreciate the dialog.

I would like to keep Typescript syntax. Please, advise if that must be changed.

It isn't required (none of them are).
But it could reduce the amount of abstraction between what you write and what gets run, which can be easier to debug.

I had some difficulties with node and jest as well. Uvu did the job, I will consider to replace later

Yes, Jest is becoming somewhat notorious for incomplete/broken ESM support (though no fault of their own, they have upstream dependencies blocking them).
Vitest is what I'd recommend if you enjoy the Jest style of structuring tests https://vitest.dev/
node:test for a lighter weight tap like approach to testing.

Not for now, but if you can clarify more I will consider

What would you like clarified?
As a general rule of thumb, build artifacts should not be checked into source control.
Source control is for source code.
Storing artifacts is what CI/CD servers and package managers are for.

A few other things:

  1. There are types for the markdown constructs provided in @types/mdast, they don't need to be manually defined from unist https://github.com/bchainhub/remark-corepass/blob/f9d5f56f69b63c9d2b6315d4398b815c4625da37/src/index.ts#L10-L24
  2. When parsing markdown, the plugin will be passed a @types/mdast Root node https://github.com/bchainhub/remark-corepass/blob/f9d5f56f69b63c9d2b6315d4398b815c4625da37/src/index.ts#L63 using this will reduce the number of casts you need to access node properties, as they already have the correct type.
  3. Adding TypeScript doc comments /** doc comment */ to the options https://github.com/bchainhub/remark-corebc/blob/a972c7e37257f37ea0234262981d8b670a304257/src/index.ts#L5-L18 describing each, and preserving the comments in the output https://github.com/bchainhub/remark-corebc/blob/a972c7e37257f37ea0234262981d8b670a304257/tsconfig.json#L10 would allow IDEs like VSCode to show the documentation inline as people are typing an option.
  4. When module is set to node16 https://github.com/bchainhub/remark-corebc/blob/a972c7e37257f37ea0234262981d8b670a304257/tsconfig.json#L4-L8 the moduleResolution and esModuleInterop options can be removed
  5. Up to you how far back you want to support. Node LTS only maintains Node 18+ at the time of writing https://github.com/bchainhub/remark-corebc/blob/a972c7e37257f37ea0234262981d8b670a304257/package.json#L58 https://github.com/nodejs/Release?tab=readme-ov-file#release-schedule
  6. The module references unist https://github.com/bchainhub/remark-corebc/blob/a972c7e37257f37ea0234262981d8b670a304257/src/index.ts#L1 but it is not defined as a dependency in package.json https://github.com/bchainhub/remark-corebc/blob/a972c7e37257f37ea0234262981d8b670a304257/package.json#L41-L44. That works in npm, but will break pnpm and yarn

@rastislavcore
Copy link
Contributor Author

All your points are valid. Thank you @ChristianMurphy for your feedback, it will increase the quality of the code.

  1. This part was most tricky to develop to avoid situations like: Property 'children' does not exist on type 'Node'. and similar. But when I will find better solution I may change it.
  2. Sorry, this part is not clear to me. Is it possible to give me little example? Is it related to previous point?
  3. I like to have clean code as much as possible. Anyway you got point. TypeScript compiler's removeComments option doesn't provide granular control. It either removes all comments or none. I assume wit the tiny scripts like those will be readme enough.
  4. I am trying to reach as minimum as possible, but keeping stable.
  5. I assume @types/unist should be added as devDependencies. These types are only used during development for type-checking and are not needed at runtime. Please, correct me when I am wrong.

I'd recommend git ignoring generated files like the dist and types folders, first time contributors tend to directly edit those instead of modifying the source files. Git ignoring generated removes that confusion.

I understand this part, but in there is CDN delivery services based on GitHub, which I found sometimes quite useful. Then I am keeping it. But I am open for discussion.

Overall I would like to thank you, you are skilled developer willing to help other projects.

@remcohaszing
Copy link
Member

remcohaszing commented Mar 2, 2024

A remark plugin written in TypeScript should roughly have the following layout:

import { Root } from 'mdast'
import { Plugin } from 'unified'
import { visit } from 'unist-util-visit';

export interface RemarkCorepassOptions {
  /**
   * Describe this option here.
   */
  enableIcanCheck?: boolean;

  /**
   * Describe this option here.
   */
  enableSkippingIcanCheck?: boolean;
}

/**
 * Describe your plugin here.
 */
const remarkCorepass: Plugin<[RemarkCorepassOptions?], Root> = (options = {}) => {
  return (ast) => {
    visit(ast, 'text', (node, index, parent) => {

    })
  }
}

export default remarkCorepass

This gives you type inference for pretty much everything you need.

There is no need for the types directory. Instead, enable "declaration": true in tsconfig.json.

Packages in the unified ecosystem currently support Node.js 16 and greater, so you can’t guarantee support for a lower version.

For an example of a fairly minimal remark plugin authored in TypeScript, see remark-mdx-frontmatter.

@ChristianMurphy
Copy link
Member

But when I will find better solution I may change it.
Sorry, this part is not clear to me. Is it possible to give me little example? Is it related to previous point?

See Remco's example above.
In addition https://unifiedjs.com/learn/recipe/tree-traversal-typescript/ and https://unifiedjs.com/learn/recipe/narrow-node-typescript/ can provide insights on how to get the correct types.

I assume with the tiny scripts like those will be readme enough.

A surprising number of adopters do not read the readmes, and instead go straight to trying to use the plugin.

I assume @types/unist should be added as devDependencies. These types are only used during development for type-checking and are not needed at runtime. Please, correct me when I am wrong.

That is not enough for plug and play dependency systems like yarn and pnpm, it needs to be in dependencies or peerDependencies.
I'd recommend using dependencies.
Stay away from peerDependencies if possible, those get handled different across package managers and even between versions of the same package manager, which can cause unexpected and difficult to debug situations.

I understand this part, but in there is CDN delivery services based on GitHub, which I found sometimes quite useful. Then I am keeping it.

There are even more CDN services surrounding npm.
https://www.jsdelivr.com/, https://unpkg.com/, https://www.skypack.dev/, and https://esm.sh/ to name a few.

If the concern is serving the most recent and up-to-date version.
The previous recommendation of:

TypeScript JSDoc which can run without a compile step https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

will serve you better, as it can deliver the raw source file from a PR or branch, without a compile step which may or may not have been run.

@rastislavcore
Copy link
Contributor Author

Thank you folks @ChristianMurphy @remcohaszing for your advices. Highly appreciated!

I tried to implement all your advices. When is something outstanding or some failure, please feel free to comment. I am really enjoying what you point out. Makes sense.

@ChristianMurphy
Copy link
Member

@rastislavcore the types would be a lot simpler if you use @types/mdast instead of re-declaring everything 🙂

Some other thoughts

  1. the fediverse plugin documents "The plugin scans for Fediverse user notations (e.g., @username@domain) in your markdown content and transforms them into markdown links." but trying with a fediverse handle mentioned in the readme does not do anything https://stackblitz.com/edit/github-ajuwfm?file=src%2Fmain.ts
  2. remark-corebc crashes with "Error: Cannot handle unknown node delete" when following the instructions from the readme https://stackblitz.com/edit/github-cnzcy8?file=package.json,src%2Fmain.ts
  3. remark-corepass also crashes with "Error: Cannot handle unknown node delete" when following the instructions from the readme https://stackblitz.com/edit/github-2vdudu?file=package.json,src%2Fmain.ts

@remcohaszing
Copy link
Member

You may also want to look into mdast-util-find-and-replace.

@wooorm
Copy link
Member

wooorm commented Mar 6, 2024

@ChristianMurphy is this ✅ for you now?

@ChristianMurphy
Copy link
Member

@wooorm I won't block if others are comfortable.

I'm not really yet, it doesn't work as documented #1291 (comment)
And the types are off #1291 (comment)

@wooorm
Copy link
Member

wooorm commented Mar 6, 2024

@rastislavcore are you planning on working on these points listed by Christian?

@rastislavcore
Copy link
Contributor Author

Sorry folks, I was quite busy. Will look today.

@rastislavcore
Copy link
Contributor Author

I highly apologize for long delay gents. I had to take another project in the meantime.

Anyway the update is:

  • delete is replaced with text node and negation sign due to incompatibility
  • In the fediverse case I extend support of the plain detection. The case was that only "mailto:" link was accepted - de facto script work after the transformation only. One more note here: domain/subdomain detection is simple, because script should catch more advanced domain structures - such as emoji or ENS

@ChristianMurphy is it now ✅ ?

@ChristianMurphy
Copy link
Member

Hey @rastislavcore! 👋

TL;DR Progress! One seems to mostly work as documented now! Two don't seem to work as documented still, and all three have typings which appear to be off/broken. More on that below:


The types are still broken for all three #1291 (comment)


The plugins still don't seem to do what their readmes say they do.
For example

remark-corebc says

This Remark plugin, "remark-corebc," transforms Core Blockchain notations into markdown links

It doesn't crash now (progress!), but it also doesn't produce a link https://stackblitz.com/edit/github-cnzcy8-vrj5mk?file=src%2Fmain.ts

The readme should also probably link to the specific Core Blockchain you support.
There are a lot of blockchain-ish thinks called Core.


Similar with CorePass

This Remark plugin, "remark-corepass," is designed to transform CorePass notations into markdown links

It is being transformed, but the generated content is not a link https://stackblitz.com/edit/github-2vdudu-xtnnmx?file=package.json,src%2Fmain.ts


remark-fediverse-user does seem to work generating links now ✅
https://stackblitz.com/edit/github-ajuwfm-vryent?file=src%2Fmain.ts

Though as noted above, the types are still psuedo-mdast, which poses risk to version upgrades and use with other plugins https://github.com/bchainhub/remark-fediverse-user/blob/cc1d3a631240306aa872d01653782948fa52b164/src/index.ts#L4-L31

@rastislavcore
Copy link
Contributor Author

Thank you, @ChristianMurphy, for the notes. They effectively improve the quality and security of the code.

Changes:

  • Implemented mdast
  • Improved documentation

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @rastislavcore!

  1. @types/unist is no longer needed as a dependency https://github.com/bchainhub/remark-corebc/blob/bb441ad221834d347bdfbdae41ed9633ff4d5bb3/package.json#L42
  2. @types/mdast is needed as a dependency (not devDependency) https://github.com/bchainhub/remark-corebc/blob/bb441ad221834d347bdfbdae41ed9633ff4d5bb3/package.json#L46
  3. There is a merge conflict here, in this PR that needs to be resolved

rastislavcore and others added 2 commits April 29, 2024 19:56
Signed-off-by: Rastislav ₡ORE <[email protected]>
Signed-off-by: Rastislav ₡ORE <[email protected]>
@rastislavcore
Copy link
Contributor Author

Thank you folks!

@ChristianMurphy I am getting from tests:

Segmentation fault (core dumped)
Error: Process completed with exit code [13]

https://github.com/remarkjs/remark/actions/runs/8883162589/job/24389434308?pr=1291#step:5:14

I assume something with tests itself? Is it?

@ChristianMurphy
Copy link
Member

Core dump is the usually a message GHA prints if a job is cancelled.
That job was cancelled because the other had already failed.
The error from the logs from the actual failing job https://github.com/remarkjs/remark/actions/runs/8883162589/job/24389434308?pr=1291#step:5:19

> ./packages/remark-cli/cli.js . --frail --output --quiet && prettier . --log-level warn --write && xo --fix

doc/plugins.md: written
118:3 warning Unexpected unordered list marker `-`, expected `*` unordered-list-marker-style remark-lint
120:3 warning Unexpected unordered list marker `-`, expected `*` unordered-list-marker-style remark-lint

⚠ 2 warnings

@rastislavcore
Copy link
Contributor Author

Right. But why is it failing? 😅 Seems like all is fine now. I did do the correction.

@ChristianMurphy
Copy link
Member

It looks like in your latest PR there's a different issue

> build
> tsc --build --clean && tsc --build && type-coverage

Segmentation fault (core dumped)

That is coming from TypeScript and Node 22 not working together.
Usually that's due to native/compiled code being included that links to outdated Node APIs, but I don't think TypeScript has any native code 🤔
It may be a bug in Node itself 🤔

@rastislavcore
Copy link
Contributor Author

It looks like in your latest PR there's a different issue

> build
> tsc --build --clean && tsc --build && type-coverage

Segmentation fault (core dumped)

That is coming from TypeScript and Node 22 not working together. Usually that's due to native/compiled code being included that links to outdated Node APIs, but I don't think TypeScript has any native code 🤔 It may be a bug in Node itself 🤔

Maybe then we should skip the tests because fix can be in next Node version.
Or optionally downgrade Node version.
You decide. In one of my previous projects I was facing similar issue, then I was forced to downgrade for a time beeing till there was fix.

@ChristianMurphy
Copy link
Member

@wooorm @remcohaszing thoughts?

@wooorm
Copy link
Member

wooorm commented Apr 30, 2024

Well that’s new. Is this TS 5.5 on Node 22?

No, the latest TS is 5.4.5, released 2 weeks ago. And last week this worked: https://github.com/remarkjs/remark/actions/runs/8814565179/job/24194685030

@wooorm
Copy link
Member

wooorm commented Apr 30, 2024

I do get this locally, on Node 22.0.0, with the same TS 5.4.5, on macOS.

What fails is:

$ ./node_modules/.bin/tsc --build
Segmentation fault: 11

@wooorm
Copy link
Member

wooorm commented Apr 30, 2024

@remcohaszing any knowledge of how best to debug TS to figure out where in the CLI things are going wrong? So we can figure out whether this is more an issue for TS or for Node?

@remcohaszing
Copy link
Member

I’m troubleshooting by focusing on just remark-parse now.

I figured this might be related to that we’re doing some TypeScript things wrong, as I tried to explain in https://github.com/orgs/unifiedjs/discussions/238. However, that doesn’t solve it.

I figured it could be related to types in JSDoc, but having only a .ts file doesn’t solve it.

Neither tsc nor tsc --showConfig crashes, only tsc --build does.

Downgrading to Node.js 20 solves it. I suggest we do this for the time being.

@wooorm
Copy link
Member

wooorm commented Apr 30, 2024

Circling back here. It looks like this issue affects all many repos. See microsoft/TypeScript#58369 (comment).

@wooorm
Copy link
Member

wooorm commented Apr 30, 2024

I locked the build to Node 20, rebasing to main should side-step this issue for now.

@rastislavcore
Copy link
Contributor Author

Hi guys, unfortunately here are some conflicts, should I investigate them?

More:

image

@ChristianMurphy
Copy link
Member

That is GitHub's merge+rebase feature.
@wooorm is referring to pulling/rebasing locally, where you can resolve any conflicts.

https://github.com/git-guides/git-pull

@wooorm
Copy link
Member

wooorm commented Apr 30, 2024

Can’t you click “Update branch” here on this PR? And then pull locally?
I don’t care about merge commits in your PR.
We’ll squash and make it all into one clean commit!

@wooorm wooorm changed the title Update plugins.md Add some plugins to list of plugins May 2, 2024
@wooorm wooorm merged commit cb8fdd2 into remarkjs:main May 2, 2024
2 checks passed

This comment has been minimized.

@wooorm wooorm added 📚 area/docs This affects documentation 💪 phase/solved Post is done labels May 2, 2024
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label May 2, 2024
@rastislavcore rastislavcore deleted the patch-1 branch May 2, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

5 participants