Skip to content
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

feat: embed other mdx/md files #22

Merged
merged 12 commits into from
Apr 9, 2021
Merged

Conversation

aleclarson
Copy link
Collaborator

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
Copy link
Collaborator Author

aleclarson commented Apr 5, 2021

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
Copy link
Collaborator Author

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

@aleclarson
Copy link
Collaborator Author

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

@aleclarson
Copy link
Collaborator Author

OK, ready to merge :)

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"
..and cache any resolved imports.
Parsed and compiled MDX files will be reused between importers (and when an importer changes) if they have not changed.
@brillout
Copy link
Owner

brillout commented Apr 6, 2021

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
Copy link
Collaborator Author

@brillout That is correct!

@brillout
Copy link
Owner

brillout commented Apr 6, 2021

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

@aleclarson
Copy link
Collaborator Author

aleclarson commented Apr 6, 2021

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
Copy link
Collaborator

ajstrand commented Apr 6, 2021

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
Copy link
Owner

brillout commented Apr 7, 2021

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 :-).

This makes the core plugin easier to grok.
@aleclarson
Copy link
Collaborator Author

aleclarson commented Apr 7, 2021

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..

  1. improve the core experience of MDX
  2. 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
Copy link
Owner

brillout commented Apr 8, 2021

@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
Copy link
Collaborator Author

aleclarson commented Apr 8, 2021

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
Copy link
Owner

brillout commented Apr 8, 2021

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
Copy link
Owner

brillout commented Apr 8, 2021

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
Copy link
Collaborator

ajstrand commented Apr 9, 2021

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

@aleclarson
Copy link
Collaborator Author

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

@brillout
Copy link
Owner

brillout commented Apr 9, 2021

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

👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants