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

Refactor to generate JS AST #1382

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Refactor to generate JS AST #1382

merged 1 commit into from
Dec 18, 2020

Conversation

wooorm
Copy link
Member

@wooorm wooorm commented Dec 15, 2020

This PR changes the internals of the core @mdx-js/mdx package to generate a JavaScript syntax tree instead of a string. This fixes escaping issues such as #1219. It makes mdx-hast-to-jsx much more palatable. It also prevents several Babel parses. It paves the way for passing in Babel plugins, which is useful for users, but also for us to compile to React.createElement, _jsx, or Vue’s h calls directly and make MDX’s output directly usable.

  • babel-plugin-apply-mdx-type-props: add parentType (used to be when serializing in mdx)
  • mdx: use rehype-minify-whitespace to remove superfluous whitespace
  • mdx: use hast-util-to-estree to transform hast to estree
  • mdx: use estree-to-babel to transform estree to Babel
  • mdx: generate estree/Babel instead of strings
  • mdx: use @babel/generator to serialize Babel AST
  • vue: stop supporting the react transform: (it doesn’t make sense)
  • vue: fix support for props to components

Related to GH-741.
Related to GH-1152.

Closes GH-606.
Closes GH-1028.
Closes GH-1219.

@wooorm wooorm added the 🗄 area/interface This affects the public interface label Dec 15, 2020
@vercel
Copy link

vercel bot commented Dec 15, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mdx/mdx/47du63gcf
✅ Preview: Failed

[Deployment for 04a85f7 failed]

@@ -1,348 +1,312 @@
const {transformSync} = require('@babel/core')
Copy link
Member Author

Choose a reason for hiding this comment

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

it might be easier to compare this file side by side. Because a lot changed

export default function createMdxElement(type, props, ...children) {
let node

export default function createMdxElement(type, props, children) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I made it pretty hard for myself last time by thinking it’d be wise to also support the React babel transform, for Vue.
That was stupid. Now our Vue runtime only supports the Vue babel transform.

@JounQin
Copy link
Member

JounQin commented Dec 15, 2020

So does this mean that we can reuse the AST in eslint-mdx directly in the future? That's amazing!

@wooorm
Copy link
Member Author

wooorm commented Dec 15, 2020

Correct!

I now implemented it in the compiler that does hast -> string. But I think we should expose it in the future on the interface of the main mdx package

@wooorm
Copy link
Member Author

wooorm commented Dec 15, 2020

@wooorm wooorm mentioned this pull request Dec 15, 2020
@JounQin
Copy link
Member

JounQin commented Dec 16, 2020

You can already do so with hast-util-to-estree btw:

syntax-tree/hast-util-to-estree@c54a908/test.js#L750-L769

@wooorm I tried this and find there is no comments/tokens/loc/range info in the transformed AST which are required by ESLint, can they be exposed or should I get them manually in other ways.

@wooorm
Copy link
Member Author

wooorm commented Dec 16, 2020

HTML comments are supported in hast-util-to-estree. JS comments are not supported in acorn.
For token, I don’t know. How do they work?
loc is added on certain things.
range: similar to loc but offset based (so wel also have it for certain things), so also PRs welcome

@JounQin
Copy link
Member

JounQin commented Dec 16, 2020

HTML comments are supported in hast-util-to-estree. JS comments are not supported in acorn.
For token, I don’t know. How do they work?
loc is added on certain things.
range: similar to loc but offset based (so wel also have it for certain things), so also PRs welcome

@wooorm

According to https://github.com/acornjs/acorn/blob/master/acorn/src/options.js#L117-L122, I got tokens by the following:

const doc = `
<>
  {/* MDX is awesome! */}
  <div
    // MDX is awesome!
    style={{ color: "white", backgroundColor: "black", padding: "24px 32px" }}
  ></div>
</>;
`;

const comments = [];
const tokens = [];

const mdast = fromMarkdown(doc, {
  extensions: [
    mdxjs({
      acornOptions: {
        locations: true,
        ranges: true,
        onComment: comments,
        onToken: tokens,
      },
    }),
  ],
  mdastExtensions: [mdxFromMarkdown],
});

tokens are available here now, but not comments.

Acorn itself is working:

let acorn = require("acorn");
const jsx = require("acorn-jsx");

acorn = acorn.Parser.extend(jsx());

const comments = [];

acorn.parse(doc, {
  onComment: comments,
});

console.log(comments);

// output
// [
//   { type: 'Block', value: ' MDX is awesome! ', start: 7, end: 28 },
//   { type: 'Line', value: ' MDX is awesome!', start: 41, end: 59 }
// ]

@wooorm
Copy link
Member Author

wooorm commented Dec 16, 2020

@JounQin One problem though is tokens and positional info from markdown, as your example is just JSX

@wooorm
Copy link
Member Author

wooorm commented Dec 17, 2020

@JounQin Also, comments are rather non-standard in estree. Different parsers add them differently. ESLint supports the comments array on Programs. But Babel and recast do not, so that’s not something I can solve right now (coderaiser/estree-to-babel#3)

@JounQin
Copy link
Member

JounQin commented Dec 17, 2020

@wooorm We don't need comments on AST, but we should be able to detect comments on parsing like onComment of acorn.

@wooorm
Copy link
Member Author

wooorm commented Dec 17, 2020

We don't need comments on AST, but we should be able to detect comments on parsing like onComment of acorn.

That is correct for ESLint, but incorrect for other tools

This PR changes the internals of the core `@mdx-js/mdx` package to generate a
JavaScript syntax tree instead of a string.
This fixes escaping issues such as #1219.
It makes `mdx-hast-to-jsx` much more palatable.
It also prevents several Babel parses.
It paves the way for passing in Babel plugins, which is useful for users, but
also for us to compile to `React.createElement`, `_jsx`, or Vue’s `h` calls
directly and make MDX’s output directly usable.

* `babel-plugin-apply-mdx-type-props`: add `parentType`
* `mdx`: use `rehype-minify-whitespace` to remove superfluous whitespace
* `mdx`: use `hast-util-to-estree` to transform hast to estree
* `mdx`: use `estree-to-babel` to transform estree to Babel
* `mdx`: generate estree/Babel instead of strings
* `mdx`: use `@babel/generator` to serialize Babel AST
* `vue`: stop supporting the react transform: (it doesn’t make sense)
* `vue`: fix support for props to components

Related to GH-741.
Related to GH-1152.

Closes GH-606.
Closes GH-1028.
Closes GH-1219.
@vercel vercel bot temporarily deployed to Preview December 18, 2020 08:53 Inactive
@wooorm wooorm merged commit 3783554 into main Dec 18, 2020
@wooorm wooorm deleted the no-strings-attached branch December 18, 2020 15:50
wooorm added a commit that referenced this pull request Dec 18, 2020
This PR changes the internals of the core `@mdx-js/mdx` package to generate a
JavaScript syntax tree instead of a string.
This fixes escaping issues such as #1219.
It makes `mdx-hast-to-jsx` much more palatable.
It also prevents several Babel parses.
It paves the way for passing in Babel plugins, which is useful for users, but
also for us to compile to `React.createElement`, `_jsx`, or Vue’s `h` calls
directly and make MDX’s output directly usable.

* `babel-plugin-apply-mdx-type-props`: add `parentType`
* `mdx`: use `rehype-minify-whitespace` to remove superfluous whitespace
* `mdx`: use `hast-util-to-estree` to transform hast to estree
* `mdx`: use `estree-to-babel` to transform estree to Babel
* `mdx`: generate estree/Babel instead of strings
* `mdx`: use `@babel/generator` to serialize Babel AST
* `vue`: stop supporting the react transform: (it doesn’t make sense)
* `vue`: fix support for props to components

Related to GH-741.
Related to GH-1152.

Closes GH-606.
Closes GH-1028.
Closes GH-1219.
Closes GH-1382.

Reviewed-by: Christian Murphy <[email protected]>
@wooorm
Copy link
Member Author

wooorm commented Dec 20, 2020

@JounQin Looks like this can attach comments: https://github.com/davidbonnet/astravel#astravelattachcommentsast-comments--ast, that makes it much easier than having to figure it out myself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface
3 participants