Skip to content

Conversation

@ChristopherBiscardi
Copy link
Member

Fixes #307

Pulls the layout for an MDX file into a reference that will be stable
across renders. Previously, a layout defined in MDX content as an
inline arrow function:

export default ({ children, ...props }) => <div {...props}>{children}</div>

would result in function defined inline in render, causing the
reference to be created each time resulting in a remounting.

This PR stablizes the reference by pulling the layout outside of
render, resulting in no remount.

@vercel
Copy link

vercel bot commented Nov 10, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@ChristopherBiscardi
Copy link
Member Author

The additional declaration isn't making it through the compilation process. Since I tested this approach on already-compiled code it didn't have that issue when I tested originally.

I've also since discovered that a couple of things rely on the raw MDX output, like the runtime, so this would require a bit more change than anticipated.

@ChristopherBiscardi
Copy link
Member Author

This approach passes the tests but still needs to be expanded so that the same thing happens in the runtime as well. buble handles classes, so this should be straightforward and I plan to do it for this PR.

@johno
Copy link
Member

johno commented Nov 14, 2018

This is looking great, thanks!

We're definitely reaching a point where we might want to think about how to handle the MDX "templating" since it's starting to get pretty logic-filled. I wonder if we should just duplicate some of the output logic but have it handled differently based on whether skipExport is true or not?

@ChristopherBiscardi
Copy link
Member Author

I definitely agree that this templating is getting a bit hard to work with. I messed up a quote, etc here or there a couple times. I'll finish up the full conversion and then see how it looks, then change it if it still needs changing

Copy link
Contributor

@silvenon silvenon left a comment

Choose a reason for hiding this comment

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

Maybe an inline snapshot test would help, to verify that everything rendered correctly: .toMatchInlineSnapshot()

@ChristopherBiscardi
Copy link
Member Author

@silvenon I added a snapshot test with the other tests in mdx/test/index.test.js. This seemed like the right place since both mdx and runtime packages have explicit tests against the JSX output. I don't know how to get rid of the wrapped quotes in the inline output though. Any ideas?

@silvenon
Copy link
Contributor

I didn't realize there would be so much MDX content, it would be better if this was a regular snapshot, because it takes a lot of space.

What do you mean by wrapped quotes?

@ChristopherBiscardi
Copy link
Member Author

it would be better if this was a regular snapshot, because it takes a lot of space.

Can also use a smaller content. I re-used a fixture that already existed.

What do you mean by wrapped quotes?

By wrapped quotes, I means this effect. Where there's a template string, then the string of code starts with double quotes leading to escaped doubled quotes all over the code.

.toMatchInlineSnapshot(`
"import { Baz } from \\"./Fixture\\";
import { Buz } from \\"./Fixture\\";
...

@silvenon
Copy link
Contributor

Can also use a smaller content. I re-used a fixture that already existed.

Yeah, maybe it's better if you just use a single # Hello World. It should be enough to monitor this simple output.

By wrapped quotes, I means this effect. Where there's a template string, then the string of code starts with double quotes leading to escaped doubled quotes all over the code.

The string itself may not be pretty to look at, but try to mess up the output and see the diff when you run the test. If the diff is nice, that's all that matters. 😉

Copy link
Contributor

@silvenon silvenon left a comment

Choose a reason for hiding this comment

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

Looks nice!

Fixes mdx-js#307

Pulls the layout for an MDX file into a reference that will be stable
across renders. Previously, a layout defined in MDX content as an
inline arrow function:

```js
export default ({ children, ...props }) => <div {...props}>{children}</div>
```

would result in function defined inline in render, causing the
reference to be created each time resulting in a remounting.

This PR stablizes the reference by pulling the layout outside of
render, resulting in no remount.

try class approach

works, but need to make it so that the runtime also has the fix
@ChristopherBiscardi
Copy link
Member Author

rebased. Only significant change was the additional prettier dep causing a conflict in mdx/package.json. Will squash/merge after tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants