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

Vue, rebased #1029

Merged
merged 9 commits into from Apr 27, 2020
Merged

Vue, rebased #1029

merged 9 commits into from Apr 27, 2020

Conversation

@ChristopherBiscardi
Copy link
Member

ChristopherBiscardi commented Apr 23, 2020

continuation of #1008 , having been rebased, tests fixed up, and example app working

@vercel
Copy link

vercel bot commented Apr 23, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mdx/mdx/lph6c4i4f
Preview: https://mdx-git-fork-christopherbiscardi-vue-rebased.mdx.now.sh

@codebender828
Copy link
Contributor

codebender828 commented Apr 26, 2020

Thank you so much @ChristopherBiscardi.

Really great work with this! Sorry I saw this 3 days later. I've been doing school projects during the previous week. Glad you were able to help with this! Were the @vue/testu-utils tests for the Vue MDXComponent component able to run as well?

I only saw the vue-loader tests in the PR test checks.

Screen Shot 2020-04-26 at 6 11 33 PM

examples/vue/package.json Outdated Show resolved Hide resolved
examples/vue/package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview Apr 27, 2020 Inactive
packages/vue-loader/package.json Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview Apr 27, 2020 Inactive
packages/vue-loader/package.json Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview Apr 27, 2020 Inactive
packages/vue/package.json Outdated Show resolved Hide resolved
@johno
johno approved these changes Apr 27, 2020
Copy link
Member

johno left a comment

😍 Amazing @codebender828 and @ChristopherBiscardi!!!

@johno johno merged commit 47b2c62 into mdx-js:master Apr 27, 2020
7 checks passed
7 checks passed
Build and Test on macOS-latest with Node.js 10
Details
Build and Test on macOS-latest with Node.js 12
Details
Build and Test on ubuntu-latest with Node.js 10
Details
Build and Test on ubuntu-latest with Node.js 12
Details
Build and Test on windows-latest with Node.js 10
Details
Build and Test on windows-latest with Node.js 12
Details
now Deployment has completed
Details
@codebender828
Copy link
Contributor

codebender828 commented Apr 29, 2020

🔥🔥🔥 So much life

@johno johno mentioned this pull request Apr 30, 2020
3 of 22 tasks complete
@wooorm
Copy link
Member

wooorm commented May 1, 2020

Does anyone in the combining Vue + React world know if there’s a difference in how props must be cased on HTML elements? For example, React accepts className, htmlFor, and things like strokeMiterlimit (lower l). Does Vue require the exact same casings?
As casing is not defined by JSX but by React, I’m wondering if this thus creates discrepancies per framework?

@sw-yx
Copy link
Contributor

sw-yx commented May 2, 2020

congrats team! is the vue stuff documented anywhere? interested to take a look. cc @sdras

@johno
Copy link
Member

johno commented May 2, 2020

It's not currently documented, but #851 is the issue tracking docs. Don't have an exact estimate on when the docs will be added (right now it's sort of a "hidden" feature until we further test it and will become officially supported in v2).

Also, we do have an example app in examples that you can check out in the meantime.

@johno
Copy link
Member

johno commented May 2, 2020

@wooorm I'm not familiar with a mapping between React and Vue differences and tried to google it a bit (I'm not well-versed in React). We'll prolly want to create a mapping library for any differences since serialization might differ quite drastically between libraries.

@codebender828
Copy link
Contributor

codebender828 commented May 2, 2020

@wooorm I think might have an idea of what you're trying to achieve. In Vue 2 the render function attributes are spread out into attrs, props, and domProps. So it looks something like this:

The h is Vue's createElement.

h('div', {
  class: ['foo', 'bar'],
  style: {
    color: 'red'
  },
  attrs: {
    id: 'foo'
  },
  domProps: {
    innerHTML: ''
  },
  on: {
    click: foo
  },
  key: 'foo',
  ref: 'chakra'
}, this.$slots.default)

In Vue 3, the plan is to flatten the createElement signature. So it will look something like this.

h('div', {
  class: ['foo', 'bar'],
  style: { color: 'red' },
  id: 'foo',
  innerHTML: '',
  onClick: foo,
  key: 'foo',
  ref: 'chakra'
}, this.$slots.default)

In any case, I think @johno 's suggestion would be good to create a map between MDX props to Vue props. The Vue 2 props could be mapped to Vue 3 props as well in the @mdx-js/vue-loader depending on the consumer's Vue version.

@wooorm
Copy link
Member

wooorm commented May 2, 2020

Some more info here: https://github.com/vuejs/jsx/tree/dev/packages/babel-plugin-transform-vue-jsx#difference-from-react-jsx.

It’s a bit unfortunate that JSX is different based on whether it’s for React or Vue (even with Vue 3, as innerHTML or class for example won’t work)

@codebender828
Copy link
Contributor

codebender828 commented May 2, 2020

@wooorm . So it seems. The Vue core team seems to be working a JSX implementation of for Vue 3 but I also cannot guarantee that it will have the same signature as Vue 2.X.

https://github.com/vuejs/vue-next#official-libraries-vue-3-support-status

I should also clarify that I don't have enough information about v3's JSX plan. Perhaps someone on the core team could shed some light on whether there will be significant changes to v3's JSX implementation.

If the JSX signatures also has big differences between v2.X and v3, then we can always drop down to the render function layer. It might be simpler to map the render function objects 🙏🏽

@wooorm
Copy link
Member

wooorm commented May 2, 2020

The default currently (assuming v2) is to pass things to components directly in, so if a user passes a ‘props’ object in, we won’t touch it. So everything should work, except that react mdx and vue mdx are different. We can document that we don’t support Vue’s Babel transform (and be open to a pull request for that if there’s interest?)

“Normal” elements (as in, coming from markdown) are different (think remark or rehype transforms, but also the language flag on code, or the task-list-item class on checklists), these are currently not put into a specific attrs object. In the parser rewrite I did add support for hast-to-hyperscript, which can detect vue or react, and magically switch based on that. However, that’s currently hard coded for React and should be updated. Does the compiler have “knowledge” of whether it’s compiling to React or Vuecurrently?

@codebender828
Copy link
Contributor

codebender828 commented May 2, 2020

Does the compiler have “knowledge” of whether it’s compiling to React or Vuecurrently?

I don't think the compiler is aware of what framework it's compiling to. It just spits out some JSX code. Since Vue doesn't have the JSX pragma support we need to expose the render function within that JSX context block where we call MDXContent().

},
render(createElement) {
h = mdx.bind({ createElement, components: this.components })
return MDXContent({ components: this.components })
}

It's not very elegant, but it works. I'm not the best JSX expert, but we made a render function that was "aware" of the elements returned by mdx.

https://github.com/mdx-js/mdx/blob/master/packages/vue/src/create-element.js

In the parser rewrite I did add support for hast-to-hyperscript, which can detect vue or react, and magically switch based on that.

If this magical switch is possible, then I'm all for it 😄

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.