Skip to content

Conversation

@ChristianMurphy
Copy link
Member

@ChristianMurphy ChristianMurphy commented Nov 23, 2021

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

related to mdx-js/.github#16

this applies the same approach, moving sandbox examples into the repository for easier maintenance and versioning.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Nov 23, 2021
@ChristianMurphy ChristianMurphy requested review from a team November 23, 2021 17:41
@ChristianMurphy ChristianMurphy added the 📚 area/docs This affects documentation label Nov 23, 2021
Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

  • would be nice to start using async/await in all these examples
  • would be nice of this is matching the code style that all other projects have for JS, prettier + xo

@wooorm
Copy link
Member

wooorm commented Nov 23, 2021

Is prettier set up in this repo? Here’s the config if not: https://github.com/unifiedjs/collective/blob/b6d6fc9ae19643a7a982f4fc0f3dc11b9be9eb1a/package.json#L25-L39. And I typically have *.md and *.html in a .prettierignore

@wooorm
Copy link
Member

wooorm commented Nov 23, 2021

Stackblitz can also be enabled with just a link right? In that case, 👍, better to not favor one particular vendor!

@ChristianMurphy ChristianMurphy force-pushed the sandbox-starters-in-repo branch from e2f4d44 to f35e86d Compare November 23, 2021 18:02
Comment on lines +46 to +48
| remark only (for markdown to markdown) | [codesandbox](https://codesandbox.io/s/github/remarkjs/.github/tree/main/sandbox-templates/remark-with-parcel) | [stackblitz](https://stackblitz.com/github/remarkjs/.github-remark/tree/main/sandbox-templates/remark-with-parcel) |
| remark and rehype (for markdown to html) | [codesandbox](https://codesandbox.io/s/github/remarkjs/.github/tree/main/sandbox-templates/remark-rehype-with-parcel) | [stackblitz](https://stackblitz.com/github/remarkjs/.github-remark/tree/main/sandbox-templates/remark-rehype-with-parcel) |
| react-markdown | [codesandbox](https://codesandbox.io/s/github/remarkjs/.github/tree/main/sandbox-templates/react-markdown-with-create-react-app) | [stackblitz](https://stackblitz.com/github/remarkjs/.github-remark/tree/main/sandbox-templates/react-markdown-with-create-react-app) |
@ChristianMurphy
Copy link
Member Author

Added prettier and xo.
Updated list of links to a table with description, codesandbox, and stackblitz

@wooorm
Copy link
Member

wooorm commented Nov 23, 2021

build fails!

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

one nit.
Did you see my earlier comment on maybe using async/await?

@ChristianMurphy
Copy link
Member Author

build fails!

XO warnings resolved

@ChristianMurphy
Copy link
Member Author

Did you see my earlier comment on maybe using async/await?

Parcel doesn't support top level await yet parcel-bundler/parcel#4028
An async IIFE could be used, but I don't have a strong preference on that 🤷‍♂️

@wooorm
Copy link
Member

wooorm commented Nov 23, 2021

Examples in remark packages now use a main(), referencing an async function main() { ... }. You can catch that and display the error. And inside it, use await?

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

fine!

@ChristianMurphy
Copy link
Member Author

Examples in remark packages now use a main(),

updated to use an async main function

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Good to have a top-level place to catch errors. I think it’s weird to not have it, but have it wrapping the await, so that it catches some errors but not others. How about this for the samples?

}
}

main()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
main()
main().catch((errror) => {
document.querySelector('#error').textContent = String(error)
})

Copy link
Member Author

Choose a reason for hiding this comment

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

Main itself has a try catch block for error catching.
I'm not sure there are any errors left from main for this to catch.

Copy link
Member

Choose a reason for hiding this comment

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

The function body is currently wrapped in a try/catch. I’m assuming that people are going to change the function body and introduce errors outside the try/catch, resulting in uncaught exceptions

Comment on lines +25 to +38
try {
const file = await unified()
.use(remarkParse)
// Add any remark plugins here
.use(remarkRehype)
// Add any rehype plugins here
.use(rehypeStringify)
.process(sourceMarkdown)

document.querySelector('#result').contentWindow.document.body.innerHTML =
String(file)
} catch (error) {
document.querySelector('#error').textContent = error
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
try {
const file = await unified()
.use(remarkParse)
// Add any remark plugins here
.use(remarkRehype)
// Add any rehype plugins here
.use(rehypeStringify)
.process(sourceMarkdown)
document.querySelector('#result').contentWindow.document.body.innerHTML =
String(file)
} catch (error) {
document.querySelector('#error').textContent = error
}
const file = await unified()
.use(remarkParse)
// Add any remark plugins here
.use(remarkRehype)
// Add any rehype plugins here
.use(rehypeStringify)
.process(sourceMarkdown)
document.querySelector('#result').contentWindow.document.body.innerHTML =
String(file)

Copy link
Member Author

Choose a reason for hiding this comment

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

🤷‍♂️ try {} catch {} reads cleaner than .then and .catch chaining. It would feel weird to make the example less clear 🤔

@wooorm wooorm changed the title docs: move sandbox examples into the repository Add sandboxes to repo Nov 24, 2021
@wooorm wooorm merged commit d4971fd into remarkjs:main Nov 24, 2021
@github-actions
Copy link

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

@ChristianMurphy ChristianMurphy deleted the sandbox-starters-in-repo branch November 24, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📚 area/docs This affects documentation 🤞 phase/open Post is being triaged manually

Development

Successfully merging this pull request may close these issues.

2 participants