Closed rastislavcore closed 6 months ago
Hi! It seems some of the things asked in the template are missing? Please edit your post to fill out everything.
You won’t get any more notifications from me, but I’ll keep on updating this comment, and remove it when done!
Thanks, — bb
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 100.00%. Comparing base (
a185821
) to head (9216116
). Report is 4 commits behind head on main.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@ChristianMurphy Good catches!
Mostly all of them integrated.
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:
@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@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./** 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.module
is set to node16
https://github.com/bchainhub/remark-corebc/blob/a972c7e37257f37ea0234262981d8b670a304257/tsconfig.json#L4-L8 the moduleResolution
and esModuleInterop
options can be removedunist
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
All your points are valid. Thank you @ChristianMurphy for your feedback, it will increase the quality of the code.
Property 'children' does not exist on type 'Node'.
and similar. But when I will find better solution I may change it.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.
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
.
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.
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.
@rastislavcore the types would be a lot simpler if you use @types/mdast
instead of re-declaring everything 🙂
Some other thoughts
delete
" when following the instructions from the readme https://stackblitz.com/edit/github-cnzcy8?file=package.json,src%2Fmain.tsdelete
" when following the instructions from the readme https://stackblitz.com/edit/github-2vdudu?file=package.json,src%2Fmain.tsYou may also want to look into mdast-util-find-and-replace
.
@ChristianMurphy is this ✅ for you now?
@wooorm I won't block if others are comfortable.
I'm not really yet, it doesn't work as documented https://github.com/remarkjs/remark/pull/1291#issuecomment-1977099187 And the types are off https://github.com/remarkjs/remark/pull/1291#issuecomment-1973943421
@rastislavcore are you planning on working on these points listed by Christian?
Sorry folks, I was quite busy. Will look today.
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@ChristianMurphy is it now ✅ ?
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 https://github.com/remarkjs/remark/pull/1291#issuecomment-1973943421
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
Thank you, @ChristianMurphy, for the notes. They effectively improve the quality and security of the code.
Changes:
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?
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
Right. But why is it failing? 😅 Seems like all is fine now. I did do the correction.
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 🤔
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.
@wooorm @remcohaszing thoughts?
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
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
@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?
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.
Circling back here. It looks like this issue affects ~all~ many repos. See https://github.com/microsoft/TypeScript/issues/58369#issuecomment-2085053231.
I locked the build to Node 20, rebasing to main should side-step this issue for now.
Hi guys, unfortunately here are some conflicts, should I investigate them?
More:
That is GitHub's merge+rebase feature. @wooorm is referring to pulling/rebasing locally, where you can resolve any conflicts.
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!
Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/*
labels.
Initial checklist
Description of changes
Added 3 plugins into documentation: