Skip to content

Conversation

@ChristopherBiscardi
Copy link
Member

closes #319

@vercel
Copy link

vercel bot commented Nov 20, 2018

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

@johno
Copy link
Member

johno commented Nov 20, 2018

Thanks @ChristopherBiscardi, can you update the snapshots as well?

@ChristopherBiscardi
Copy link
Member Author

ah right, of course. Updated.

@silvenon
Copy link
Contributor

I wish we could add a test proving that the warning is no longer there, i.e. a one which would show the warning before applying the patch. But that doesn't sound trivial.

Thanks for spotting and patching this so quickly!

@silvenon silvenon merged commit 398ee4c into mdx-js:master Nov 20, 2018
@silvenon
Copy link
Contributor

Released!

@ChristopherBiscardi
Copy link
Member Author

I also wish we could test for that, but I'm more confused as to why it didn't show up before that in our tests. It wasn't just a warning, it was an Uncaught ReferenceError that blocked rendering.

@johno
Copy link
Member

johno commented Nov 20, 2018

Yeah I'm actually quite surprised this got through the test suite. I'll do a bit of digging today to see how we can get a more thorough integration test wired up to make sure we get an ❌if core outputs something that can't render.

@johno
Copy link
Member

johno commented Nov 20, 2018

I would've expected at least @mdx-js/runtime to error, though in the core tests for @mdx-js/mdx we're mostly testing parseability and never actually rendering (which we should change).

@johno
Copy link
Member

johno commented Nov 20, 2018

#321 should do it: https://travis-ci.org/mdx-js/mdx/jobs/457690098#L606

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.

v16: super before this access

3 participants