-
Notifications
You must be signed in to change notification settings - Fork 140
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: upgrade mdx-js to v2 #164
Conversation
Co-authored-by: Salakar <[email protected]>
@@ -28,7 +28,7 @@ describe('hydration', () => { | |||
const result = readOutputFile('basic', 'index') | |||
|
|||
// server renders correctly | |||
expect(result).toMatch( | |||
expect(result.replace(/(\r\n|\n|\r)/gm, '')).toContain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Rendered output now includes nextjs header scripts etc and various line breaks - so this test I've change to do a contains
check instead.
@@ -170,7 +170,7 @@ describe('serialize', () => { | |||
}) | |||
|
|||
test('strips imports & exports', async () => { | |||
const result = await renderStatic(`import foo from 'bar' | |||
const result = await renderStatic(`import foobar from 'bar' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: To be able to remove the imports it now needs to be able to successfully parse the JS, but it can't since there's 2x foo
constant assignments - therefore renamed the import on the test.
Fantastic - thanks so much for the contribution! We'll check this out and see if we can get it released as a beta perhaps 🎉 |
FYI this PR has been released as |
Is this working with Next v12 for you @Salakar? I'm having a ton of trouble getting MDX2 working. MDX1 doesn't work for my usecase. |
] | ||
|
||
const compiledMdx = await mdx(source, { ...mdxOptions, skipExport: true }) | ||
const compiledMdx = mdx.sync(source, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a contributor (yet) but chiming in - We Probably want to keep this as async
This PR contains all the necessary changes to upgrade
mdx-js
to the latest v2 version with tests passing. It may not be accepted yet(?) but we thought it's worth sending up as a reference to yourselves/others wanting to try it out.Co-authored by @Ehesp and @Salakar