-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add more options to playground, directives, format, etc #2295
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportPatch and project coverage have no change.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2295 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 2177 2177
=========================================
Hits 2177 2177 ☔ View full report in Codecov by Sentry. |
BTW @wooorm adding this md option is useful for me to better understand how the md parser works. Currently I'm facing a little Docusaurus issue where html tags are removed (or sanitized) from the output. For example: ## HTML
### Styling
<span style="color: blue;">blue span</span>
<p style="color: green;">green p</p>
<button style="color: red;">red button</button>
<div style="border: solid; background-color: lime; padding: 10px">
lime <span style="color: red; margin: 10px;">red</span>
</div>
### Embeds
### Images
#### Closed image tag:
<img src="/img/docusaurus.png"/>
#### Unclosed image tag:
<img src="/img/docusaurus.png">
### Iframe
<iframe src="/"/>
### Security
```md
<p>
When pressing this button, no alert should be printed
<button onClick="alert('unsafe');">Click me</button>
</p>
```
<p>
When pressing this button, no alert should be printed
<button onClick="alert('unsafe');">Click me</button>
</p> https://deploy-preview-8960--docusaurus-2.netlify.app/tests/pages/markdown-tests-md I tried to use some options like remarkRehypeOptions: {
allowDangerousHtml: true,
passThrough: ['div', 'button', 'img', 'iframe', 'html'],
}, But it doesn't seem to work, see facebook/docusaurus#8960 Any idea how to allow arbitrary html tags? So far it seems only Also wondering about the default behavior: considering MDX is a language and already unsafe, doesn't it make sense to allow arbitrary tags in a similar unsafe way when compiling I mean, it makes sense to me that we get the same safety level for My feeling is that filtering by default most html tags will lead to many users opening issues on the Docusaurus repo 🤪 |
docs/_component/editor.client.js
Outdated
<a href="https://mdxjs.com/packages/mdx/#optionsformat"> | ||
<code>format: 'md'</code> | ||
</a> | ||
</label> |
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.
Maybe we should add a tooltip here to explain MDX infers this based on the input file extension by default. I think this option makes sense for the playground, but it’s best to discourage using this option.
Actually, maybe a radio button is more appropriate here than a checkbox, although the checkbox currently fits nice in the UI.
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.
yes a radio or select can also work, open to change a bit the UI, not sure how to organize now that it's not just a list of remark plugins.
but it’s best to discourage using this option.
Why? Docusaurus uses MDX and I think supporting a commonmark mode can help existing docs sites to adopt it more easily, ie to have something less strict running asap without a ton of compilation errors to fix
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.
yes a radio or select can also work, open to change a bit the UI, not sure how to organize now that it's not just a list of remark plugins.
Let’s wait for @wooorm’s feedback, as I think he designed the website. I think it looks great already. :)
but it’s best to discourage using this option.
Why? Docusaurus uses MDX and I think supporting a commonmark mode can help existing docs sites to adopt it more easily, ie to have something less strict running asap without a ton of compilation errors to fix
This option changes whether files are treated as commonmark or MDX. Leaving it off, means this is inferred from the file extension. It’s not about strictness (although basically anything is technically valid markdown, but MDX has syntax errors). This option is useful when the file extension is unknown (such as in this playground), but I wouldn’t recommend it otherwise. I don’t know how Docusaurus uses it exactly though.
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.
On my side, I find it helpful to understand the MDX compiler behavior when handling .md files.
I'm not really saying the format
option is useful or recommended, and would be fine if instead we had the ability to customize the file name for example, as long as I can easily get the AST for an input parsed with CommonMark compat.
Although it's not recommended, Docusaurus historically has many sites that use MDX with a .md
extension. One of the first unexpected things people stumble upon while upgrading to MDX 2 is that some tags (images, videos... ) do not appear anymore. See feedback here: facebook/docusaurus#8469 (reply in thread)
For this reason, it's likely I'll add an option format: 'mdx'
, to allow our users to upgrade their MDX version without having to rename all their files at the same time.
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.
You can pass the filename. compile()
takes a VFile compatible. In fact, I recommend always passing a filename if possible. Some plugins may depend on it. For example the JSX dev runtime depends on it.
await compile({
value: '# Hello <strong>world</strong>!',
path: '/home/me/hello.mdx'
})
I would also be cautious treating .md
files as MDX. MDX treats certain constructs differently.
I.e. given the following content:
<div>
foo
bar
</div>
In markdown, this is equivalent to the following HTML:
<div> foo bar </div>
In MDX, this is equivalen to the following JSX:
<div>
<p>foo</p>
<p>bar</p>
</div>
I understand it’s useful to add the option for migration purposes in Docusaurus.
it’s best to discourage using this option.
I think this may have been misinterpreted. I means it’s best to discourage using this option in general, but there are definitely legitimate use cases. The playground, where there are no actual file names, is a good use case for such an option.
docs/_component/editor.client.js
Outdated
if (!config.position) { | ||
removePosition(file.data.mdast, {force: true}) | ||
removePosition(file.data.hast, {force: true}) | ||
removePosition(file.data.esast, {force: true}) |
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.
BTW not sure this works for esast, is there a tool to remove esast positions to make exploring the AST more convenient?
I mean these nodes:
{
start: 68,
end: 75,
loc: {
start: {
line: 4,
column: 13,
offset: 68
},
end: {
line: 4,
column: 20,
offset: 75
}
}
}
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.
You can try unist-util-position-from-estree
.
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.
@remcohaszing that only gets a position, it doesn’t clear start/end/loc.
@slorber I don’t think there is one, in some tests I’ve used a walker/visit and called delete
for those fields
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.
Replaced with:
import {visit as visitEstree} from 'estree-util-visit'
export function removePositionEsast(tree) {
visitEstree(tree, remove)
return tree
function remove(node) {
delete node.loc
delete node.start
delete node.end
delete node.range
}
}
If
Tags are allowed. In MDX. But then they are JSX. You are not using the MDX language. You are using the markdown language. Which supports raw strings of HTML inside it. No framework (react/preact/vue/whatever) understands strings of HTML, they don’t know what to do with them, to illustrate with a common example: The point of MDX is to actually parses JSX and markdown combined. If you want to support HTML in plain markdown, are happy to include more code, and to do more work, then you can use mdx/packages/mdx/test/compile.js Lines 733 to 746 in e77be3a
|
docs/_component/editor.client.js
Outdated
<a href="https://mdxjs.com/packages/mdx/#optionsformat"> | ||
<code>format: 'md'</code> | ||
</a> | ||
</label> |
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.
I think a radio toggle like this is better: https://wooorm.com/markdown-tm-language/, https://github.com/wooorm/markdown-tm-language/blob/656e7dea3f5248c728a6eeda4e9780778bd6449f/example/index.jsx#L412-L437. Don’t “connect” it to an option. But allow people to choose whether the input is plain markdown or MDX
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.
Co-authored-by: Titus <tituswormer@gmail.com> Signed-off-by: Sébastien Lorber <slorber@users.noreply.github.com>
Thanks for the pointers @wooorm , will study all that and see what I'm able to do to support html nodes in Docusaurus .md files ;) Handled some of the review comments |
Thanks @wooorm, was able to make it work in Docusaurus with rehype-raw + passThrough of MDX nodes (some remark plugins like admonition are producing those) I added remark-raw as a playground option too because I think it can help understand better how it all works: Note: the latest version of rehype-raw@6 still uses hast-util-raw@7 while latest version is hast-util-raw@8 so I didn't get your enhanced MDX error message (syntax-tree/hast-util-raw@8e7f703) |
Yep, soon! Always takes time to go through the entire ecosystem ;) |
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.
Some small nits, then ready to go! Appreciate your continued work!
docs/_component/editor.client.js
Outdated
@@ -25,34 +29,56 @@ lowlight.registerLanguage('js', javascript) | |||
lowlight.registerLanguage('json', json) | |||
lowlight.registerLanguage('md', markdown) | |||
|
|||
export function removePositionEsast(tree) { |
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.
- Can you remove the
export
? Don’t think that’s needed - Can you stick with the style of using calls first, definitions later (somewhere at the bottom probably!)
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.
removed the export, typo
Not sure what you mean by "calls first", isn't it already the case? I used the code style found in unist-util-remove-position
function removePositionEsast(tree) {
visitEstree(tree, remove)
return tree
function remove(node) {
delete node.loc
delete node.start
delete node.end
delete node.range
}
}
Or is it the top-level fn declaration that I should put at the bottom of the file?
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.
Yep, I mean, place it in the highest scope, at the bottom!
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.
Done 👍
But It was already at the highest scope, and useMdx() is at the top. So we have one helper fn above and under the default exported component 🤪
Updated the PR and fixed the merge conflicts Handled the review, apart the "calls first, defs later" the rest looks good 👍 Let me know if I should change anything else |
Sweeet, thanks for your continued work! |
Awesome, thank you!! |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@mdx-js/loader](https://mdxjs.com) ([source](https://github.com/mdx-js/mdx)) | [`2.3.0` -> `3.0.0`](https://renovatebot.com/diffs/npm/@mdx-js%2floader/2.3.0/3.0.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@mdx-js%2floader/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@mdx-js%2floader/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@mdx-js%2floader/2.3.0/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@mdx-js%2floader/2.3.0/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | | [@mdx-js/react](https://mdxjs.com) ([source](https://github.com/mdx-js/mdx)) | [`2.3.0` -> `3.0.0`](https://renovatebot.com/diffs/npm/@mdx-js%2freact/2.3.0/3.0.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@mdx-js%2freact/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@mdx-js%2freact/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@mdx-js%2freact/2.3.0/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@mdx-js%2freact/2.3.0/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>mdx-js/mdx (@​mdx-js/loader)</summary> ### [`v3.0.0`](https://github.com/mdx-js/mdx/releases/tag/3.0.0) [Compare Source](https://github.com/mdx-js/mdx/compare/2.3.0...3.0.0) (see <https://mdxjs.com/migrating/v3/> on how to migrate) ##### Change - [`e08b759`](https://github.com/mdx-js/mdx/commit/e08b7596) [`5afa48e`](https://github.com/mdx-js/mdx/commit/5afa48e6) Change to require Node 16 - [`5a13d73`](https://github.com/mdx-js/mdx/commit/5a13d73b) Change to use export maps - [`cbc2822`](https://github.com/mdx-js/mdx/commit/cbc2822f) Update `unified`, types, plugins, etc - [`96b51f9`](https://github.com/mdx-js/mdx/commit/96b51f93) Remove inferral of development from `NODE_ENV` ##### Change (unlikely to affect you) - [`c961af8`](https://github.com/mdx-js/mdx/commit/c961af80) Remove `useDynamicImport` option - [`9cb26fd`](https://github.com/mdx-js/mdx/commit/9cb26fd1) `@mdx-js/register`: remove package - [`0d1558a`](https://github.com/mdx-js/mdx/commit/0d1558a3) `@mdx-js/esbuild`: remove experimental `allowDangerousRemoteMdx` - [`0f62bce`](https://github.com/mdx-js/mdx/commit/0f62bce9) `@mdx-js/node-loader`: remove `fixRuntimeWithoutExportMap` - [`4f92422`](https://github.com/mdx-js/mdx/commit/4f924227) `@mdx-js/preact`: remove deprecated `MDXContext`, `withMDXComponents` - [`a362bb4`](https://github.com/mdx-js/mdx/commit/a362bb43) `@mdx-js/react`: remove deprecated `MDXContext`, `withMDXComponents` ##### Add - [`e12f307`](https://github.com/mdx-js/mdx/commit/e12f3079) Add support for passing `baseUrl` when running - [`2c511a4`](https://github.com/mdx-js/mdx/commit/2c511a40) Add support for `baseUrl` as a `URL` - [`1863914`](https://github.com/mdx-js/mdx/commit/1863914c) Add deprecation warning for classic runtime - [`a34177c`](https://github.com/mdx-js/mdx/commit/a34177c3) Add support for ES2024 in MDX, adjacent JSX and expression blocks - [`44fd9ca`](https://github.com/mdx-js/mdx/commit/44fd9cac) Add support for `await` in MDX - [`3a7f194`](https://github.com/mdx-js/mdx/commit/3a7f1947) Add `tableCellAlignToStyle` option, to use `align` - [`fdfe17b`](https://github.com/mdx-js/mdx/commit/fdfe17b8) `@mdx-js/rollup`: add support for Vite development mode by [@​remcohaszing](https://github.com/remcohaszing) in [mdx-js/mdx#2376 ##### Misc - [`f48d038`](https://github.com/mdx-js/mdx/commit/f48d038b) Remove unneeded pragma comment after transform - [`8f3b292`](https://github.com/mdx-js/mdx/commit/8f3b2920) Add a `use strict` directive to function bodies - [`172e519`](https://github.com/mdx-js/mdx/commit/172e5190) `@mdx-js/react`: fix to classify `@types/react` as a peer dependency by [@​remcohaszing](https://github.com/remcohaszing) in [mdx-js/mdx#2281 - [`a7bd79b`](https://github.com/mdx-js/mdx/commit/a7bd79bb) Refactor output to immediately export default - [`e525db9`](https://github.com/mdx-js/mdx/commit/e525db9b) [`dae82ae`](https://github.com/mdx-js/mdx/commit/dae82ae4) Refactor some errors - [`ce173f2`](https://github.com/mdx-js/mdx/commit/ce173f28) Refactor to add types for JSX runtimes - [`8a56312`](https://github.com/mdx-js/mdx/commit/8a563128) Refactor output to use spread, not `Object.assign` by [@​remcohaszing](https://github.com/remcohaszing) in [mdx-js/mdx#2328 - [`825717f`](https://github.com/mdx-js/mdx/commit/825717fd) Refactor to sort default components by [@​remcohaszing](https://github.com/remcohaszing) in [mdx-js/mdx#2318 - [`d8a62d2`](https://github.com/mdx-js/mdx/commit/d8a62d20) Add missing type dependencies by [@​arcanis](https://github.com/arcanis) in [mdx-js/mdx#2256 ##### Docs - [`a9f0c04`](https://github.com/mdx-js/mdx/commit/a9f0c046) Add guide on injecting components - [`24e3d8d`](https://github.com/mdx-js/mdx/commit/24e3d8d1) Add compat sections to readmes - [`30e4a5d`](https://github.com/mdx-js/mdx/commit/30e4a5d5) Add sponsor - [`07503a5`](https://github.com/mdx-js/mdx/commit/07503a5f) Update link to KaTeX CSS in docs by [@​victor23k](https://github.com/victor23k) in [mdx-js/mdx#2360 - [`74aee56`](https://github.com/mdx-js/mdx/commit/74aee569) [`bc1d9e5`](https://github.com/mdx-js/mdx/commit/bc1d9e56) [`765310c`](https://github.com/mdx-js/mdx/commit/765310ca) [`6d1e64d`](https://github.com/mdx-js/mdx/commit/6d1e64d9) Refactor docs - [`7fd1d9a`](https://github.com/mdx-js/mdx/commit/7fd1d9a4) Fix docs on how to use solid by [@​BeiyanYunyi](https://github.com/BeiyanYunyi) in [mdx-js/mdx#2300 - [`4129f90`](https://github.com/mdx-js/mdx/commit/4129f90e) Fix a couple typos by [@​deining](https://github.com/deining) in [mdx-js/mdx#2266 - [`bb902f8`](https://github.com/mdx-js/mdx/commit/bb902f83) Fix typo by [@​ChristianMurphy](https://github.com/ChristianMurphy) in [mdx-js/mdx#2380 ##### Site - [`78a1eb5`](https://github.com/mdx-js/mdx/commit/78a1eb52) Add v3 blog post - [`2b1948c`](https://github.com/mdx-js/mdx/commit/2b1948c0) Add v3 migration guide - [`d6bb70c`](https://github.com/mdx-js/mdx/commit/d6bb70ca) Add improved error display in playground - [`89097e4`](https://github.com/mdx-js/mdx/commit/89097e4c) Remove unmaintained dev-dependency - [`3e23ba9`](https://github.com/mdx-js/mdx/commit/3e23ba90) Add more options to playground - [`d92128b`](https://github.com/mdx-js/mdx/commit/d92128ba) Fix links in docs - [`ab3aa96`](https://github.com/mdx-js/mdx/commit/ab3aa966) Add GitHub pages by [@​remcohaszing](https://github.com/remcohaszing) in [mdx-js/mdx#2377 - [`a2c8693`](https://github.com/mdx-js/mdx/commit/a2c86936) Fix site by [@​wooorm](https://github.com/wooorm) in [mdx-js/mdx#2358 - [`dbe9f44`](https://github.com/mdx-js/mdx/commit/dbe9f44b) Fix playground AST views w/ clone by [@​Jokcy](https://github.com/Jokcy) in [mdx-js/mdx#2315 - [`7504cfb`](https://github.com/mdx-js/mdx/commit/7504cfb8) Add more options to playground, directives, format, etc by [@​slorber](https://github.com/slorber) in [mdx-js/mdx#2295 - [`57301df`](https://github.com/mdx-js/mdx/commit/57301dfa) Add resizable editor/layout to playground by [@​slorber](https://github.com/slorber) in [mdx-js/mdx#2296 - [`9eb747d`](https://github.com/mdx-js/mdx/commit/9eb747d6) Add info on how to build site locally by [@​slorber](https://github.com/slorber) in [mdx-js/mdx#2297 **Full Changelog**: mdx-js/mdx@2.3.0...3.0.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/X-oss-byte/Nextjs).
This PR adds new useful options to have in the playground:
:::warning
Note: I think it's better to not have positions in the AST by default, this creates useless noise and I doubt people use positions in the playground anyway.