feat!: experimentally support remark-mdx@2 #284
Conversation
|
@wooorm All standard es features are working as expected, but jsx nodes are not passed as estree like format, it that possible to parse it by enabling |
|
I'm on a phone right now (sorry, can't release, will soon) But you need something along the lines of https://github.com/wooorm/xdm/blob/main/lib/core.js#L67, up until (including) rehype-recma, if you want an estree. That's what you want, right?! |
|
@wooorm I tried like following, but it seems not working. export const mdProcessor = unified().use(remarkParse).freeze()
export const getEsLintMdxProcessor = (tokens: AST.Token[]) =>
mdProcessor()
.use(
remarkMdx,
// FIXME: wrong typing of `remark-mdx`
// @ts-ignore
{
acornOptions: {
locations: true,
ranges: true,
onToken: tokens,
},
},
)
.use(remarkMarkAndUnravel)
.use(remarkRehype, {
allowDangerousHtml: true,
passThrough: MDX_NODE_TYPES as any,
})
.use(() => toEstree)
.freeze()import { Basic } from './basic'
<Basic></Basic>[
{
"type": "ImportDeclaration",
"start": 0,
"end": 31,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 31
}
},
"range": [
0,
31
],
"specifiers": [
{
"type": "ImportSpecifier",
"start": 9,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 9
},
"end": {
"line": 1,
"column": 14
}
},
"range": [
9,
14
],
"imported": {
"type": "Identifier",
"start": 9,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 9
},
"end": {
"line": 1,
"column": 14
}
},
"range": [
9,
14
],
"name": "Basic"
},
"local": {
"type": "Identifier",
"start": 9,
"end": 14,
"loc": {
"start": {
"line": 1,
"column": 9
},
"end": {
"line": 1,
"column": 14
}
},
"range": [
9,
14
],
"name": "Basic"
}
}
],
"source": {
"type": "Literal",
"start": 22,
"end": 31,
"loc": {
"start": {
"line": 1,
"column": 22
},
"end": {
"line": 1,
"column": 31
}
},
"range": [
22,
31
],
"value": "./basic",
"raw": "'./basic'"
}
}
] |
|
And it seems |
|
Here's an explanation of the steps: https://github.com/wooorm/xdm#architecture. You first need to go from mdast -> hast, before being able to go hast -> estree |
Maybe I understand it, but is there any wrong thing at #284 (comment)?
|
|
I don't know why, but |
|
@JounQin I’m guessing it’s this: https://github.com/unifiedjs/unified#description. await pipeline.run(pipeline.parse(tree, file), file) |
|
@wooorm Thank you for clarify, and I got the following this time
|
|
Can you post the code you’re using? |
|
|
|
|
|
|
OK, next problems:
So maybe I need to rewrite |
Where isn’t it added? Please explain in more detail.
This sounds wrong. That’s not what MDX is. It’s not a literal. |
|
OK. I don't know if it would be a problem for other jsx rules, let's handle it in the future. |
|
How does that interfere with ESLint? What breaks? The bigg problem with It’s impossible: we can either add the actual raw but break ESLint, or we can add a fake raw, probably preventing ESLint from working. |
|
You know more about ESLint that I do, but quickly going through the ESLint rules, I’m not seeing |
https://github.com/eslint/eslint/search?q=node.raw&type= It's used in many rules, and it breaks on my tests. |
V2 of the MDX format is a breaking change. You glossed over my explanation. |
I understand it is a breaking change, I'm just asking for
I'm not quite sure to understand, can you provide some codes so that I can change it locally at https://github.com/mdx-js/eslint-mdx/blob/next-debug/packages/eslint-mdx/src/plugins/hast-util-to-estree.js to make sure if it is working. |
|
Maybe this’ll help. Here’s an explanation for JSX: acornjs/acorn-jsx#105. |
How could I get this raw input at https://github.com/syntax-tree/hast-util-to-estree/blob/f381bc45a33a7ad0e6b0b136bece824b652421f6/index.js#L382? I change it to So I want to get raw first, if there are problems we can fix it in the future. |
|
Another problem, I can't get all tokens from |
|
That’s similar. To illustrate: what |
No, it's markdown, not es. I don't think eslint-mdx should care. But it's children like That's why I want markdown in jsx to be |
|
So how does ESLint work with partial tokens? As in, “holes”? For lots of things, it can’t have tokens. Such as Regardless, |
|
@wooorm I think it's time to go ahead, you can try to release a |
|
You mean that this is ready for release? anyway, really cool that you’ve made this work :) |
@wooorm Ready for a beta |
|
The current implementation is heavily depending on With |
There was a problem hiding this comment.
Nice! Really cool that a bunch of hacky code could be removed.
I do have a couple questions!
One more “soft” change in MDX 2, that I wanted to check with you, is that now, .mdx more strongly should be used for MDX and .md for normal markdown. I wanted to make sure you are aware of that. Perhaps there could be rules for this, or you know other things that need to change here?
(for more info, see the big purple box here: https://mdxjs.com/docs/what-is-mdx/#mdx-syntax)
| * @returns A Promise that resolves to the dynamically imported module. | ||
| */ | ||
| /* istanbul ignore next */ | ||
| export const loadEsmModule = <T>(modulePath: URL | string): Promise<T> => |
There was a problem hiding this comment.
Can you replace all of this with https://github.com/wooorm/import-meta-resolve?
There was a problem hiding this comment.
@wooorm I don't think so, this function is used to hack TypeScript and call import() in cjs (TypeScript transforms import() into Promise.resolve(require()) by default and have no way to disable this behavior as the comments), I'm not sure how import-meta-resolve will resolve this problem.
Notice, the ESLint plugin package is still cjs only due to ESLint itself.
There was a problem hiding this comment.
import-meta-resolve will not solve that problem. It is also itself ESM, so that would be a problem here.
But, it’s much better at loading ESM/CJS because it supports export maps and other modern resolving features.
There was a problem hiding this comment.
So, is there anything you are proposing to change? I'm still confusing what import-meta-resolve will help with in this project. I believe native import() will handle export maps and other modern resolving features already. We're loading the module instead of resolving its absolute path here.
There was a problem hiding this comment.
If it is possible to use import-meta-resolve, I recommend doing so.
import() resolves from eslint-mdx/src/helpers.ts and imports the resolved path. (and: in your code you first try to require them. That works okay but isn’t perfect.)
import-meta-resolve resolves modules from a given URL. It is like import.meta.resolve, which is experimental in Node behind a flag.
There was a problem hiding this comment.
We're always in commonjs context here, so I believe import.meta.url is not available.
There was a problem hiding this comment.
URL is still available. In Node.js, in both ESM and CJS, you can create URLs from file paths, and file paths from URLs. import-meta-resolve/import.meta.resolve does not depend on import.meta.url.
But again: import-meta-resolve is ESM only. So if you are not in ESM here, you can’t easily use it. Though, you can create a small bundle of it:
npx esbuild import-meta-resolve --bundle --platform=node --outfile=vendor/import-meta-resolve.cjs`
Yeah, we have separated them in v1 already, and we have a flag |
|
looks good to me :) |
|
@wooorm Please help to release a |
|
As a major, I believe, right? I guess it’s good to try it out for a bit instead of just cutting 2.0.0 immediately! |
@wooorm Yeah, of course. You'll need to check whether is my script |
|
Released! I manually had to select I guess it doesn’t create a release for a prerelease 🤷♂️ Want me to add anything? |
@wooorm Just drafted https://github.com/mdx-js/eslint-mdx/releases/tag/v2.0.0-next.0 |
|
sweet, thanks! |
|
@wooorm Did you forget to run See https://unpkg.com/browse/eslint-plugin-mdx@2.0.0-next.0/ |
|
Correct, I did not run Can you:
|
|
@wooorm Sorry, I forgot about that part because we'd had |
|
What flag do I have to add to force a publish without changes? |
@wooorm https://github.com/lerna/lerna/tree/main/commands/version#--force-publish |
|
I think it worked! :) |
Thanks to synckit, we can use ESM before ESLint
close #204, close #338, close #366, close #356, close #367, close #371