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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] MDX Vue Support #239

Merged
merged 7 commits into from Nov 21, 2018
Merged

[WIP] MDX Vue Support #239

merged 7 commits into from Nov 21, 2018

Conversation

brennj
Copy link

@brennj brennj commented Aug 26, 2018

screen shot 2018-08-27 at 00 09 49

This is my attempt for needing Vue support in this wonderful project. Follows on from my issue opened here: #238

Steps taken:

  1. Implement MDXTag and MDXProvider in Vue Component land so there is something to render, and also add tests for this, these live in a new package called @mdx-js/vue-plugin-mdx.
  2. Create a custom compiler VueJSXCompiler which lives the same package, and add the tests. (A lot of the tests are kind of the same as the original compiler, which I don't know is a good thing at all. Welcome for direction here!)
  3. Add code for the loader in order to determine to output Vue or React headings. It means the loader has knowledge of the plugin which I'm assuming is a no-no, so some direction is welcome here. I also added a loader test which works fine. The loader to make Vue work is as follows:
const { VueJSXCompiler } = require('@mdx-js/vue-plugin-mdx');

...

module: {
  rules: [
    ...
    {
      test: /.mdx?$/,
      use: ['babel-loader', {
        loader: '@mdx-js/loader',
        options: {
          compilers: [VueJSXCompiler]
        }
      }],
    }
  ]
}

There is stuff still to do:

  1. Add documentation for Vue usage. (Providing the maintainers are happy of course, want to have this polished af)
  2. Add Vue example to examples (I have this ready to go locally, but it depends on yalc to install local packages - not sure how to get this running without running lerna publish yet.
  3. Run formatter, didn't do this yet! 馃槵
  4. Theres some bugs that Vue was complaining about that I still need to fix. But it does HMR on vue-cli@3.x without complaining.
  5. Figure out where I need to add peerDependencies, I'm no expert on this at all but I'm pretty sure I need it somewhere. Advice welcome.
  6. Refactor to try and remove some duplicate code if anyone sees any suggestions, feel its a lot. But plenty of test coverage to go nuts trying to reduce code.

Please let me know what you guys think!

@vercel
Copy link

vercel bot commented Aug 26, 2018

This pull request is automatically deployed with Now.

To access deployments, click Details below or on the icon next to each push.

@brennj
Copy link
Author

brennj commented Sep 3, 2018

Haven't forgotten about this to make it non-WIP! Busy last week, should get a few hours this week to get this tidied up.

Copy link
Member

@johno johno left a comment

Choose a reason for hiding this comment

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

馃殌馃寯 This is looking amazing, thanks @brennj! Would love for someone familiar with Vue to take a look, I'm pretty novice with the framework myself.

Have one comment regarding the loader, would be interested in other folks thoughts.

@@ -1,15 +1,27 @@
const { getOptions } = require('loader-utils')
const mdx = require('@mdx-js/mdx')

function getImportCode(options) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should have a separate loader specifically for Vue? I know it'd be a decent amount of duplication, but I typically prefer to keep loaders free from as much logic as possible.

Choose a reason for hiding this comment

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

I agree on this. A loader scope should be limited to one action, a option could fine tune the transpilation but not changing it scope.

@marceloavf marceloavf mentioned this pull request Sep 28, 2018
@Aaron-Pool
Copy link

Anything happening here? Love to see this merged if it's working.

@silvenon
Copy link
Contributor

silvenon commented Oct 13, 2018

Us too, but it requires a lot of rethinking in order to reduce duplication of logic. I assume that the effort paused because making high-level decisions is scary.

@johno johno changed the title [WORK IN PROGRESS] MDX Vue Support [WIP] MDX Vue Support Nov 9, 2018
@johno
Copy link
Member

johno commented Nov 21, 2018

Going to change the base branch to a new WIP vue branch so some others can pick up where @brennj has left off. Thanks so much for the initial spike!

@johno johno changed the base branch from master to vue November 21, 2018 17:38
@johno johno merged commit eb399dc into mdx-js:vue Nov 21, 2018
@Aaron-Pool
Copy link

@johno Is there a list things that still need to be done, if anyone wants to pitch in? Or is someone specific already working on finishing this?

@johno
Copy link
Member

johno commented Nov 21, 2018

Nothing specific yet. I plan on helping out as much as possible, but I am not super familiar with Vue so I could def use some help if you're interested.

@Aaron-Pool
Copy link

I'm definitely interested in helping, but I have no idea how much time I'll have. That's why I would be interested in some sort of checklist, so that I could contribute as time allows.

@Aaron-Pool
Copy link

@johno Any update here? Doesn't seem to have seen any movement in a while.

@silvenon
Copy link
Contributor

silvenon commented Jan 8, 2019

@Aaron-Pool do you know React? A code snippet of how a Vue output might look like in comparison to React would really help. This is an example snippet.

I don't know Vue so it's hard to push this feature ATM.

@Aaron-Pool
Copy link

@silvenon yeah, I think I've dabbled in react enough to be able to convert react output to the Vue equivalent. I'd be glad to give it a go, at the very least! I'll try to post something here tonight.

@Aaron-Pool
Copy link

Aaron-Pool commented Jan 9, 2019

@silvenon Ok, so I took some time to look over the example. Here are a few differences between React and Vue that are going to manifest in this example:

  • All Vue components are consumers, so the withMDXComponents HOC is unnecessary.
  • Similarly all Vue components are also providers, so the MDXProvider for Vue will just be a simple component.
  • I'm a little fuzzy on a handful of the props accepted by MDXTag, but I think it would be relatively simple to port to Vue, if I understand it correctly.

Anyways, assuming those (currently React specific) components are implemented and available, the Vue output looks mostly similar to the React output, but here's a commented conversion:

export default {
  // props have to be declared in vue, otherwise they're treated as attributes,
  // which are just string literals and not reactive
  props: {
    components: null
  },

  // The vue injection system related properties
  inject: {
    // I'm assuming this is provided by a React context, because I can't
    // actually pinpoint where it's is coming from
    layout: {
      default: null
    },
    // import 'components' from the provider hierarchy, and put it on the
    // 'this' object as 'contextComponents'. If that object isn't in the
    // provider hierchy, simply set it to an empty object
    contextComponents: {
      from: 'components',
      default: {}
    }
  },

  render() {
    // set components to the object provided as the in the component prop
    // (in view all props are placed directly on to the 'this' object, if the prop wasn't
    // given, use the component object from the provider hierarchy
    const components = this.components || this.contextComponents;
    
    return (
      <MDXTag name="wrapper" components={components}>
        <MDXTag name="h1" components={components}>{`Hello world`}</MDXTag>
        <MDXTag
          name="p"
          components={components}
        >{`Lorem ipsum dolor sit amet.`}</MDXTag>
      </MDXTag>
    );
  }
}

I'm by no means a Vue guru. But I'm pretty sure everything I've put here is accurate.

@robokozo
Copy link

I have no idea how promising this is, but I'm excited to see some movement on this topic :) Keep it up!

@raczMedia
Copy link

Pumped! Please keep going

@Aaron-Pool
Copy link

@silvenon @johno I'm still around to help out with this, with some direction. But I get it if you guys are busy with other things 馃憤

@johno
Copy link
Member

johno commented Feb 8, 2019

Thanks folks! And we'd def love some help/direction @Aaron-Pool! This is very close to the top of my TODO list, so I'm hoping to write out an issue and split it into tasks that we can share with the community sometime next week.

@jrsinclair
Copy link

This is awesome! Is there any way I can get it installed with npm into another app and try it out?

@jrsinclair
Copy link

To give further context, I'd love to see if I can get this working inside nuxt.js. If I can, I'd be happy to document how I get it working and contribute this back to the repo.

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

Successfully merging this pull request may close these issues.

None yet

9 participants