feat: embed other mdx/md files

aleclarson commented 3 years ago

By importing a .mdx or .md file without an import specifier, you can embed its contents within the importing .mdx file.

import "../foo/bar.mdx"

aleclarson commented 3 years ago

A few notes:

Imported .mdx files can import their own .mdx or .md files
Imported .mdx and .md files can have their own Remark plugins
Vite resolves the import paths, so other Vite plugins can manipulate them
Missing imports are stripped out and a warning is emitted
HMR: When an embedded file is changed, its importers will be invalidated
Embedded files have their ASTs cached in memory to improve performance when the importer is updated

aleclarson commented 3 years ago

Don't merge yet. I need to test if I'm applying Remark plugins correctly to the imported file.

aleclarson commented 3 years ago

Don't merge yet. Need to look into HMR support.

aleclarson commented 3 years ago

OK, ready to merge :)

brillout commented 3 years ago

Importing an .md file in another .md file already works; is the primary goal of the PR to make import specifiers optional?

// Currenlty already works

import Child from './child.md'

<Child/>

# Markdown

This page is written in _Markdown_.

// Equivalent to the above
// Only works after the PR is merged

import './child.md'

# Markdown

This page is written in _Markdown_.

aleclarson commented 3 years ago

@brillout That is correct!

brillout commented 3 years ago

That's neat but I'm a bit worried about the added complexity it adds to code. Thoughts?

aleclarson commented 3 years ago

I'm not worried. Why are you?

The added complexity is:

a remark plugin
a configureServer hook
a Chokidar event listener
a fork of @mdx-js/mdx's createMdxAstCompiler export
a very simple ImportMap class
a "least recently used" cache to improve performance of remark plugin

We could try to encapsulate it more, into its own src/remark-mdx-import subdirectory, but idk if it's worth the effort?

ajstrand commented 3 years ago

I think encapsulating the functionality could help, but I agree with @brillout that the change seems to add complexity with any big benefits. Adding good comments for your fork of createMdxAstCompiler could definitely help people touching the code in the future if this is going to be merged in.

brillout commented 3 years ago

I do think it's cool to be able skip the import specifier and I do like to save keystrokes. And I would definitely be up for taking it in if it were a matter of only changing a couple of LOCs.

But AFAICT the added convenience doesn't seem to justify the added complexity. For example, it would definitely decrease my motivation to maintain vite-plugin-mdx (although maybe I'm not a representative example). Or imagine we would add 10 more such feature with same complexity addition; AFAICT the code would then become a lot harder to maintain.

@aleclarson What do you think? Maybe there is a way to implement this upstream? More people would benefit from your work then :-).

aleclarson commented 3 years ago

I think the complexity is being overstated. I can maintain this feature, so there's no need for anyone else to stress themselves out over it.

Or imagine we would add 10 more such feature with same complexity addition

I don't think slippery slope applies here. Shouldn't we want features that both..

improve the core experience of MDX
are only possible thanks to Vite

..to be baked in to this plugin?

We can evaluate if future proposals meet those criteria as we go. I don't think many more will.

Maybe there is a way to implement this upstream?

With a feature as simple as this, I think it's best for vite-mdx to support it by default. I've encapsulated it into its own Vite plugin, so the complexity isn't leaking into the core plugin anymore.

brillout commented 3 years ago

@aleclarson How about you take over vite-plugin-mdx? We move it to aleclarson/vite-plugin-mdx (and you can remove my npm ownership of vite-plugin-mdx). I'm already fairly busy vite-plugin-ssr and Wildcard API anyways. You can then add as much complexity as you want ;-).

@ajstrand Would that be ok with you?

aleclarson commented 3 years ago

I don't see the need to change ownership. This is probably the last of my contributions in the foreseeable future. I'd prefer to not be the only maintainer of the entire plugin, as I don't have any free time for maintaining the parts I don't use (eg: SSR or Vue support).

brillout commented 3 years ago

Just re-read my comment and I realized that it may have come off as offensive but that wasn't my sentiment. I'm trying to find the best solution from a technical/community perspective, that's all.

I just thought you'd be willing to take over ownership, since you seemed to be the one who cares most.

brillout commented 3 years ago

I reviewed the others changes you made; LGTM.

I've encapsulated it into its own Vite plugin, so the complexity isn't leaking into the core plugin anymore.

That's makes it ok for me to merge.

@ajstrand Ok for merging?

eg: SSR

I can maintain that part.

ajstrand commented 3 years ago

@brillout I'm fine with merging the work in.

aleclarson commented 3 years ago

@brillout I've mentioned this feature in the readme. Lemme know what you think

brillout commented 3 years ago

@brillout I've mentioned this feature in the readme. Lemme know what you think

👌

brillout / vite-plugin-mdx

feat: embed other mdx/md files #22