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

feat: support MDX v2 #211

Merged
merged 34 commits into from
Feb 15, 2022
Merged

feat: support MDX v2 #211

merged 34 commits into from
Feb 15, 2022

Conversation

BRKalow
Copy link
Contributor

@BRKalow BRKalow commented Nov 18, 2021

  • Upgrading our core MDX dependencies to v2.
  • Removes esbuild integration
  • Adds optional frontmatter parsing by passing parseFrontmatter: true to serialize()
  • Adds improved error message output when an error is thrown from the MDX compiler

closes #230
closes #164

src/serialize.ts Outdated
* webpack overrides require.resolve by default and returns the module ID
* instead of the resolved path
*/
const esbuildDir = pkgDir.sync(eval('require.resolve')('esbuild'))

Choose a reason for hiding this comment

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

Lurking with zero context here, is there any way to avoid using eval? It’d be awesome to be able to use this library inside edge functions, which don’t support eval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 I'll see what I can do!

@BRKalow BRKalow marked this pull request as ready for review December 10, 2021 18:35
@artsiompeshko
Copy link

Hi! Great to see the progress on MDX2, do you have any plans for when all these changes will be ready?

@ConsoleTVs
Copy link

Will this also fix that?

trim  <0.0.3
Severity: high
Regular Expression Denial of Service in trim - https://github.com/advisories/GHSA-w5p7-h5w8-2hfq
No fix available
node_modules/trim
  remark-parse  <=8.0.3
  Depends on vulnerable versions of trim
  node_modules/remark-parse
    @mdx-js/mdx  <=2.0.0-next.8
    Depends on vulnerable versions of remark-mdx
    Depends on vulnerable versions of remark-parse
    node_modules/@mdx-js/mdx
      next-mdx-remote  <=3.0.8
      Depends on vulnerable versions of @mdx-js/mdx
      node_modules/next-mdx-remote
    remark-mdx  <=1.6.22
    Depends on vulnerable versions of remark-parse
    node_modules/remark-mdx

@jescalan
Copy link
Contributor

jescalan commented Dec 21, 2021

@artsiompeshko - It's already available if you use the tag next-mdx-remote@next -- it will be merged to stable once mdx v2 is merged to stable.

@ConsoleTVs - you are welcome to try it out and see, but unless you are directly passing user input into the library, which would be a very terrible idea, you are not affected by this vulnerability regardless

@SukkaW
Copy link

SukkaW commented Jan 15, 2022

@BRKalow @jescalan next-mdx-remote@next serialize results in a lot of spaces like:

\n       \n      \n

I've checked the source code of MDX, it sucks and I can't figure out if their compileOption includes minify or not. I am afraid that the minify has to be implemented in next-mdx-remote.

@joepio joepio mentioned this pull request Jan 31, 2022
@joepio
Copy link

joepio commented Jan 31, 2022

HTML comments seem to throw:

EDIT: Seems like an issue upstream: mdx-js/mdx#1042

<!-- I'm a comment Comment! -->
Error: [next-mdx-remote] error compiling MDX:
Unexpected character `!` (U+0021) before name, expected a character that can start a name, such as a letter, `$`, or `_` (note: to create a comment in MDX, use `{/* text */}`)

@jescalan
Copy link
Contributor

jescalan commented Jan 31, 2022

HTML comments are not allowed in mdx v2, you can use jsx comments instead - it shows an example of this in the error message

@theoludwig
Copy link

It's already available if you use the tag next-mdx-remote@next -- it will be merged to stable once mdx v2 is merged to stable.

@jescalan MDX v2.0.0 has been released stable! 🥳
See: https://mdxjs.com/blog/v2/ and https://mdxjs.com/migrating/v2/

Looking forward to have next-mdx-remote to use MDX v2. 😄

@jescalan
Copy link
Contributor

jescalan commented Feb 1, 2022

Haha nice, one hour ago. We'll get to pushing this as soon as we are able. @BRKalow any objections to merge & ship here?

@BRKalow
Copy link
Contributor Author

BRKalow commented Feb 1, 2022

@jescalan No objections! I'll give this a quick once-over this week and we'll get it merged and released.

@raulfdm
Copy link

raulfdm commented Feb 7, 2022

Is there anything missing to this release happen? 🕺

@SukkaW
Copy link

SukkaW commented Feb 11, 2022

Is there anything missing to this release happen? 🕺

Minify is missing, see #211 (comment)

@BRKalow
Copy link
Contributor Author

BRKalow commented Feb 11, 2022

@SukkaW We explicitly dropped the esbuild minification as it was causing a lot of compatibility issues when deployed to certain platforms. We do want to have some form of minification at some point, but for now we'll ship v4 without it.

You can certainly wrap serialize and minify .compiledSource in your own implementation for now!

@SukkaW
Copy link

SukkaW commented Feb 12, 2022

@SukkaW We explicitly dropped the esbuild minification as it was causing a lot of compatibility issues when deployed to certain platforms. We do want to have some form of minification at some point, but for now, we'll ship v4 without it.

I am aware that esbuild is dropped on purpose. I am just worried about minifying functionality being missing.

@max-pod
Copy link
Contributor

max-pod commented Feb 14, 2022

What's the resolution at this point? Are we awaiting the minifying functionality before pushing the merge?

@BRKalow
Copy link
Contributor Author

BRKalow commented Feb 14, 2022

What's the resolution at this point? Are we awaiting the minifying functionality before pushing the merge?

I was just wrapping up the finishing touches last Friday, will ship this week!

@BRKalow BRKalow merged commit f08d885 into main Feb 15, 2022
@BRKalow BRKalow deleted the brk.feat/mdx-v2-rc branch February 15, 2022 22:05
@theoludwig theoludwig mentioned this pull request Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to mdx 2.x
10 participants