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

AST node name changes in next.9 #1503

Closed
ChristopherBiscardi opened this issue Mar 28, 2021 · 5 comments
Closed

AST node name changes in next.9 #1503

ChristopherBiscardi opened this issue Mar 28, 2021 · 5 comments
Labels
🙉 open/needs-info This needs some more info 🐛 type/bug This is a problem

Comments

@ChristopherBiscardi
Copy link
Member

Subject of the issue

The new node types in v2-next.9 renames the types

  • export
  • import
  • jsx

into

  • mdxjsEsm
  • mdxJsxFlowElement

I believe a full list of the 5 new node types can be found here

The new node types for import and export specifically seem to mostly be additive changes (specifically, inserting the new estree AST as data on the node), but the name change will break gatsby-plugin-mdx and anyone else who is depending on the export or import nodes (as well as the jsx node).

Also note that the behavior here is slightly different. An import and an export next to each other are now combined into a single mdxjsEsm node.

Your environment

  • Packages: @mdx-js/mdx@^2.0.0-next.9

Steps to reproduce

const mdx = require("@mdx-js/mdx");

main();

async function main() {
  const t = mdx.createCompiler().parse(string);

  const f = await mdx.createCompiler().parse(`
import Thing from 'somewhere';
export const meta = {};

<Test/>
`);
  console.log(f);
}

Expected behaviour

Expected import node and export node to be separate and include one import or export per node.

Actual behaviour

both import and export are combined into a single new node type, even though the .value string still exists and would provide backwards compatibility.

{
  type: 'root',
  children: [
    {
      type: 'mdxjsEsm',
      value: "import Thing from 'somewhere';\nexport const meta = {};",
      position: [Object],
      data: [Object]
    },
    {
      type: 'mdxJsxFlowElement',
      name: 'Test',
      attributes: [],
      children: [],
      position: [Object]
    }
  ],
  position: {
    start: { line: 1, column: 1, offset: 0 },
    end: { line: 6, column: 1, offset: 65 }
  }
}

Additional questions

Is it also possible to provide backwards compatibility for the jsx node?

@ChristianMurphy
Copy link
Member

change will break gatsby-plugin-mdx and anyone else who is depending on the export or import nodes (as well as the jsx node).

True, a quick scan of https://github.com/search?l=JavaScript&p=1&q=mdx+visit+import&type=Code looking at potential impact.
Out of 12 pages of results I looked at, only four actually do something with the import node

  1. https://github.com/wilbit/docs-website/blob/02f4468da983f7b10d81704f1f00420d3c370489/codemods/buttons.js
  2. https://github.com/department-of-veterans-affairs/veteran-facing-services-tools/blob/8d39616e957cb44b6664a658b2c51dca8076fe7e/packages/documentation/plugins/gatsby-plugin-mdx(forked)/gatsby/source-nodes.js
  3. https://github.com/frontarm/mdx-util/blob/bf1e8872c50e1726665b3df769ced34144d06d41/packages/mdx.macro/mdx.macro.js
  4. https://github.com/newrelic/docs-website/blob/13cc53701d30f5f85c5f4bb2c1c03706560cddce/codemods/techTile.js

It would be nice to maintain compatibility, but if not possible, it appears not a lot of projects directly work with the MDX specific AST nodes.
It may be possible to reach out directly with GitHub issues to project to ease the transition.
That said if the node changes stay, the specification https://github.com/mdx-js/specification would need an update, and a migration guide would likely be in order.

@johno
Copy link
Member

johno commented Apr 3, 2021

At the very least, it'd be ideal if we partition/separate exports and imports, they're semantically and intentionally different.

@wooorm
Copy link
Member

wooorm commented Apr 3, 2021

The reason for having a single node that contains arbitrary JavaScript is a) in alignment with how it’s parsed, but importantly b), to allow extensions to what’s allowed. Such as var/let/const (GH-1046) without adding new nodes (so, in a minor release).

That single arbitrary JS node also allows MDX to be extended to support:

```js eval
var number = Math.PI
```

The number is {number}.

…through a plugin that walks mdast and replaces code[eval] with such an mdxjsEsm node.

Another idea is to allow more vue/svelte like syntax:

<script>
  var number = Math.PI
</script>

The number is {number}.

To answer “they're semantically [...] different” — On the JavaScript level they are. And they are separately available as a subtree. Folks can inspect and transform those subtrees, which is in my opinion preferable over trying to handle a serialized value (such as what remark-mdx-frontmatter does),
Exposing the JavaScript in a plugin mechanism is now also a minor release away in @mdx-js/mdx. This would allow arbitrary JavaScript transforms, such as minification (terser) or bundling (what mdx-bundler does currently).

@johno
Copy link
Member

johno commented Apr 3, 2021

Ah I see, I suppose we can get close to current support by exposing some APIs for walking the estree for folks that want to do things like plucking/removing imports and specific types of exports.

@wooorm
Copy link
Member

wooorm commented Oct 19, 2021

These name chagnges are intentional and honestly allow really cool things. See https://v2.mdxjs.com/docs/extending-mdx/#creating-plugins for more info on creating plugins.
Note that creating plugins is a complex space and it could definitely be improved. I’d appreciate thoughts on how to make that better.

@wooorm wooorm closed this as completed Oct 19, 2021
@wooorm wooorm added the 🙉 open/needs-info This needs some more info label Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙉 open/needs-info This needs some more info 🐛 type/bug This is a problem
Development

No branches or pull requests

4 participants