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(remix-dev): add support for handle, links & loader to mdx #3763

Merged
merged 3 commits into from
Jul 16, 2022

Conversation

TAGraves
Copy link
Contributor

@TAGraves TAGraves commented Jul 16, 2022

This adds a handle export, links export, and loader export to MDX routes, much like the existing meta and headers exports.

Closes: #2592, #2858

  • Docs
  • Tests

Testing Strategy:
I added a new integration test for mdx routes which covers the existing functionality (I believe this was uncovered before) as well as the newly added functionality.

I can see a couple different approaches for doing this. One option would be to just make the handle the frontmatter and omit the loader altogether. We could also structure the loader as:

{
  frontmatter: {/* ... */}
}

@changeset-bot
Copy link

changeset-bot bot commented Jul 16, 2022

🦋 Changeset detected

Latest commit: cea4a3e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
remix Patch
@remix-run/dev Patch
create-remix Patch
@remix-run/eslint-config Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/node Patch
@remix-run/deno Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/vercel Patch
@remix-run/architect Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 16, 2022

Hi @TAGraves,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 16, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@TAGraves TAGraves changed the title Adds support for handle, links, and loader to mdx routes feat(remix-dev): add support for handle, links, and loader to mdx Jul 16, 2022
@machour machour linked an issue Jul 16, 2022 that may be closed by this pull request
@MichaelDeBoey MichaelDeBoey changed the title feat(remix-dev): add support for handle, links, and loader to mdx feat(remix-dev): add support for handle, links & loader to mdx Jul 16, 2022
docs/guides/mdx.md Outdated Show resolved Hide resolved
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks!

@kentcdodds kentcdodds merged commit b2ef490 into remix-run:dev Jul 16, 2022
@TAGraves TAGraves deleted the tg-mdx-frontmatter-updates branch July 16, 2022 15:47
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-b2ef490-20220717 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@andreiduca
Copy link
Contributor

I wonder why is the entire attributes object exported as the loader?

export const loader = typeof attributes === "undefined" ? undefined : () => attributes;

As I understand it, in Remix the loader is a server-side function that can retrieve data, interact with the request, and even read files from the disk. This PR makes it impossible to write such loader functions inside MDX files by setting the loader to whatever static data is defined in the frontmatter attributes.

👇🏻 This is currently possible in Remix, but won't be after this PR:

---
meta:
  title: Page with dynamic content
---

import { useLoaderData } from 'remix';
import { fetchSomeData } from ...

export const loader = async () => {
  const data = await fetchSomeData();
  return { data };
};

export function ComponentUsingData() {
  const { data } = useLoaderData();

  return <div>{data.whatever}</div>;
}

# This page has dynamic content

<ComponentUsingData />

@kentcdodds
Copy link
Member

For some reason I didn't realize that would actually work. We haven't pushed this out yet as a release so I'll fix this before that happens.

@kentcdodds
Copy link
Member

Here's the fix: #3800

@kentcdodds
Copy link
Member

Actually. Thinking about it more... We already support links and handle. I'm going to revert this PR and I'll add docs for how you can do links and handle without any changes.

kentcdodds added a commit that referenced this pull request Jul 19, 2022
@kentcdodds
Copy link
Member

Ok, so links won't work yet. But loader and handle do and I just pushed a docs update. We had some issues with our website recently (also, I just realized I pushed that to dev instead of main 😅) so those will take a little while to get up, but you can find it here: https://github.com/remix-run/remix/blob/dev/docs/guides/mdx.md#advanced-example

@TAGraves
Copy link
Contributor Author

@kentcdodds Those approaches won't work for plain markdown files since they require the x in mdx 😄.

Here's a fairly simple thing we wanted to do in a layout route: take the meta.title field from our blogposts and put it in an H1 at the top of the page. Even using mdx I now need every blog post to know that the parent wants to do that and I have to include a handle or loader in every blog post to facilitate having the layout route inspect that data.

It'd be cool if we could detect if the markdown defines its own handle or loader and avoid overriding them if so. I'd be happy to take a stab at that.

@kentcdodds
Copy link
Member

kentcdodds commented Jul 19, 2022

@TAGraves, thanks for the offer. We're not planning on investing any further in our markdown/mdx support. Build-time compilation of content is not the best UX/DX and we have other priorities as well (even reviewing PRs takes time). That said I did just make a PR (#3801) to support the links export capability for mdx files (though I believe that should work for plain markdown files as well because I think we treat them the same in Remix).

Here's what I suggest. Either:

  1. Switch to using runtime compilation (with caching) via mdx-bundler. This allows you to update your content without rebuilding your whole site.
  2. Do what's being done in https://github.com/kentcdodds/remix-mdx to completely customize how your mdx gets built

We do have ideas of things we can do to more fully customize the build-time markdown/mdx experience, but that's further down the pipeline.

@kentcdodds
Copy link
Member

Oh, also I want to say thank you for your contribution here @TAGraves. I just realized that I reverted your addition to the contributors.yml file but then used the test you wrote. I'm going to add you to the other PR if that's ok with you. Normally this isn't the way we like doing open source (we really prefer giving git-credit to the people who wrote the code), I just wasn't thinking when I copy/pasted your test in my other commit. Sorry about that. And thank you for the contribution!

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

Successfully merging this pull request may close these issues.

Exporting links function doesn't work in MDX files
5 participants