Closed fracture91 closed 3 years ago
Interesting proposal! So normally you would configure mdx inside next.config.js like here, and you're missing that from this transformer right? Just want to make sure I understand.
Why shouldn't we support just sending in the object? Or perhaps both object and path to file?
"^.+\\.mdx$": [
"jest-transformer-mdx",
{ options: require("./config/mdxOptions.js") },
],
Interesting proposal! So normally you would configure mdx inside next.config.js like here, and you're missing that from this transformer right? Just want to make sure I understand.
Yup, exactly. Other MDX helpers have a similar API:
Why shouldn't we support just sending in the object? Or perhaps both object and path to file?
We could support both, but it's a potential footgun because of the JSON serialization problem with the most commonly used rehypePlugins/remarkPlugins options. From reading the source, I think these are the only possible options for mdx.sync
:
filepath
should be handled by jest-transformer-mdx
like in this PR. mdxFragment
and wrapExport
are undocumented. skipExport
has one mention in the docs for "if you have a custom use case" which I don't think applies here?
Happy to support both if that's what you'd prefer. e.g.
{ options: require("./config/mdxOptions.js") }
// or
{ options: "./config/mdxOptions.js" }
Ahh I understand, the options passed to jest get serialised. That's a shame :( Otherwise to support the rest we'd have to explicitly support each one, rather than just passing the options through. So seems like as you have it, just passing in a string to the options file path, is best.
I don't know why but it just feels a shame to support passing through a path to a file, it means that inline objects don't work. Also one has to create a new file in order to configure the transformer which is just extra work/clutter in the repo (unless you use the same file for mdx options like yourself, then there is no additional cost, but we do force developers to make their mdx options into a separate file). And thirdly one can also easily make typos to the path of the file and we'd have to handle that edge case in the transformer too (i.e. print out a helpful error message like "Can't find the path to mdx options file you passed in, got ${path}.").
This also means if one can configure mdx here, we would need to refactor the way we transpile the markdown content, currently I am just using mdx.sync
since there are no plugins. This is an issue since this function is synchronous, and if you pass in async plugins they'll just be ignored. I think it's just refactoring to be:
const jsx = await mdx(withFrontMatter, {...mdxOptions, filepath: filename})
Although in the transformer we have the same problem too, currently we export process
, but process
is synchronous, so we'll have to use/export processAsync
instead! That shouldn't be a problem, there's an example in jest's repo here (and also see https://jestjs.io/docs/next/configuration). So maybe a few more changes before letting this go through, I can take a better look when my week frees up :)
Closing in favour of https://github.com/bitttttten/jest-transformer-mdx/pull/24
Thanks! I also added two test cases to the repo, and I included support for inline objects as sometimes it does work :)
I'd like my MDX files to behave the same way in tests as they do in prod, e.g. by running all the same remark/rehype plugins. This requires
jest-transformer-mdx
to pass the same options tomdx.sync
as I do in prod.I've been using this patch successfully on my personal website, and configured it like so to read my shared test/prod MDX configuration. The config makes it into
@next/mdx
here. Here's a test that shows my plugins have the expected side effects when run throughjest-transformer-mdx
.IIRC, the reason I provide a file path in
jest.config.js
instead of an object like{remarkPlugins: [...], rehypePlugins: [...]}
is because the options must be JSON-serializable. The plugins are functions, and so they can't be serialized.require
-ing the file later insidecreateTransformer
works around that.I'm happy to add tests/docs to cover this behavior - just wanted to make sure there's interest to merge this before I do that work.