Add recma-mdx-displayname to list of plugins

[domdomegg]

Added MDX plugin to MDX plugins list in docs. Updated modified date.

Have added it as it specifically works with MDX (as per line 96). Have inserted it alpha sorted on project name (as per line 88).

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
mdx	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Jan 19, 2023 at 6:25PM (UTC)

[wooorm]

Hey hey! Cool

a) The splice at the start looks fragile! I think you can just push it to ast.body. Or unshift it. Both should work?
b) I recommend using actual ESM. Everything in unified is actual ESM. There’s nothing CJS solves here
c) recmaMdxDisplaynamePlugin -> recmaMdxDisplayname
d) return ast; is unneeded, the default is to mutate, which you do
e) the faux-esm extra default thing is unneeded if you do actual ESM
f) I personally recommend again jest, especially because you don’t care about the DOM or React in this case, it’s too much, you can use node:test in Node 16+ (Node 14 is EOL in 3 months), so that’s reasonable for a new package. It’s also probably better to check actual things instead of mocking. In this case, you can use evaluate from @mdx-js/mdx, with your plugin, and get the component back, which will have a displayName!
g) What is the reason this is needed? Are you getting warnings? AFAIK, modern browsers infer these names really well now?

[remcohaszing]

Cool plugin! I agree with the comments by @wooorm, with some additions:

1. I recommend to use ESM, but if you do choose to export CJS, the correct syntax is:

   export = recmaMdxDisplayname;

2. It would be cool if the user could generate the display name based on a custom function that receives the vfile.

   recmaPlugins: [
     [recmaMdxDisplayname, file => `MDXContent(${file.path})`]
   ]

3. As a response to g) A minifier will change mangle the function name.

[wooorm]

2. should probably somehow clean that URL into an identifier though
3. yeah, sure, maybe that’s good enough a reason, but, aren’t there options for that in minifiers? Seems better than adding display names everywhere, especially if its automated for something like this

[remcohaszing]

2. Nope, it's used in a string literal.
3. Disabling mangling could significantly decrease bundle size. Also React devtools uses the display name, which is nice :)

[wooorm]

The intention of minifying is to reduce bundle size. This prevents that. I don’t see it. I think it’s not a good idea.

[remcohaszing]

I don't see the problem? Using display name is quite a common practice in React. There's an ESLint rule to enforce it, a Babel plugin to add it, and I'm sure I've seen one to remove it in the past. HOCs tend to add it (at least when HOCs were relevant).

[wooorm]

Define problem. Nothing crashes. Just, it defeats the purpose of minifying.

common practice

Yeah, because inferring names of arrow functions didn’t exist. And stack traces were terrible in general. Now they’re good. So it isn’t really needed.

Except, hence my original question, I believe that Next.js reads displayName on pages, and requires it in some linting rule?

[domdomegg]

Thanks all for so much valuable feedback - I really appreciate all the thoughts and ideas on it! I think I've now addressed it all, but very open to discussion or any new points :)

• a) You're right! I used splice as I believed I needed it to be before the export statement. However, after some testing I've discovered you're correct - I can just use .push to add it to the end :)
• b) Thanks! I did try playing around with ESM a little when building this the first time but found it difficult to get node to play along. However, given the recommendations from both of you I tried harder at porting this to ESM, and managed to get it working. However, I then discovered that you can't use it in a Next.js project out of the box (as it uses CJS by default). I appreciate I could ask users to swap their module system for their Next config, or change their config to an async function and use import() to install this, but this seems unnecessary given compiling it as a CJS module means it's easily compatible with both CJS and ESM. So, I've decided to stick with CJS. Happy to be convinced otherwise if there are strong end-user benefits in this specific case to using ESM over CJS. Also agree that I'd just publish it as an ESM-only module if Next.js defaulted to ESM.
• c) I have made this change in the package and package name.
• d) Thanks, didn't know this. Have made this change.
• e) See (b)
• f) I tried node:test but it seemed particularly tricky to use with TypeScript in a nice cross-platform way (not that Jest is that much better, but I am at least used to its quirks). I'm also used to Jest (e.g. how to use its watch mode, how to collect coverage) and don't currently see massive benefits to switching. Will keep an eye on how node:test develops over time though and consider in future projects. I tried using the evaluate function but it requires providing arguments for Fragment, jsx and jsxs, which I'm not sure where to source from - I assume somewhere in the internals of React. I tried some mocks, and that seemed to throw an error, but with no details or stacktrace. At this stage I figured just using compile, and using it in my own projects where it works, is likely sufficient.
• g) By default, we would still have access to the function name. However, most build tools (including Next) will automatically minify names. While you can usually disable mangling variable names, doing so significantly increases the bundle size - and AFAIK there's no way to disable manglers for just specific variables. The reason I'd like to add it back just to these elements is to do things like conditional rendering on the name, so that you can add MDX-only styles for example in _app.js (demo at the bottom here, and problem explained in more detail here).

1. Thanks for teaching something new! This works much better in terms of users being able to import it with the right type definitions :) Re ESM, see (b).
2. Agreed, this would be cool. I would be wary about doing it by default (as it may be surprising to some users to have their internal file path names exposed, in case they're doing some weird remappings/imports), but I have exposed this as an option :)
3. See (g)

[remcohaszing]

Note that Next.js does support ESM (example project). I really recommend to use ESM for new projects. However, I understand migration may be hard for existing projects and you want to support those. :)

I have some good experience combining tsx for running TypeScript and c8 for code coverage. But you should use whatever you prefer for your own projects. :)

[wooorm]

Re esm: all of unified is ESM. And it’s unlikely that only your specific plugin is used. People would probably want to use other plugins too. So I don’t think it’s a burden on your users. Likely almost everyone has it anyway.

In my experience, ESM sucks at first just because it is different. You need to figure out again which things to configure and how. But once you’ve done that once or twice, it’s fine to switch.

This becomes a much bigger thing when you have a bunch of tools like Jest and TS. One of the reasons I like to keep the number of tools low myself! (Although we are using TS everywhere, albeit we don’t use .ts files).

Fragment, jsx and jsxs, which I'm not sure where to source from

All the docs have examples, expand this one here: https://mdxjs.com/packages/mdx/#optionsfragment. That shows where you can get it!

TS supports node:test if you include @types/node as a dep. What doesn’t work for you?
Also: node:test is extremely fast, I’m adding it everywhere, really recommend it!

[wooorm]

thank you! :)

[domdomegg]

Thanks, have applied suggested changes.

Thanks for the docs link, that's exactly what I was looking for. I probably could have found that myself - my bad. Will have a look at tweaking the tests later today.