framework7io / framework7-vue

Deprecated! Build full featured iOS & Android apps using Framework7 & Vue
http://framework7.io/vue/
MIT License
674 stars 151 forks source link

Using Framework7 Vue in Framework7 React #34

Closed bencompton closed 5 years ago

bencompton commented 7 years ago

@nolimits4web

Rather than manually porting the Vue components to React, I ended up writing an adapter that creates React components that leverage Framework7 Vue rendering logic and maps it to React. Check out the components in this branch to get a feel for what I'm talking about. It turns out that Vue 2.0 with template compilation ends up working very similarly to how React works, aside from some API differences. Just like React, every component is rendered via a createElement function.

The adapter isn't quite complete yet, but as you can see in the kitchen sink, several pages are implemented so far and it is working quite well aside from a few bugs that need fixing. I have Framework7 React using the Framework7 Vue NPM package, compiling it in a Gulp task, then importing the respective Vue components for each React component.

I initially started manually porting the Vue code over to React, but I quickly realized how bad of an idea it would be for long-term maintenance to have two codebases that are almost identical. With the "adapter" approach, as long as the public API of the Vue components don't change (events, tag name, etc.) and only internal component logic changes, it shouldn't require changing the React components in most cases. Some things like the integration with F7 core routing will need to be coded specifically for React, but nearly everything else can be accomplished with the adapter.

The adapter seems to work well for almost everything in Framework7 Vue. However, I have found that React is a bit more strict on things like security and data flow than Vue is. I have run into the following issues so far:

  1. React does not accept strings on style attributes (to limit XSS attacks). For example, the Progressbar component is setting translate3d style attribute via a string, and it will not work in React. When the Vue code is modified so that the translate style attribute is specified in object instead of string format, it works just fine in React. I would think that specifying styles in object format would be a best practice in Vue as well because of XSS concerns.

  2. React strongly discourages using innerHTML as strings to prevent XSS attacks. It requires using the "dangerouslySetInnerHTML" prop in React. There are several Vue components that are using domProps.innerHTML. I currently have this kind of working (you'll notice a bug on the cards kitchen sink page where an element rendered like this is missing), but I would prefer not to support it.

  3. React is pretty strict about having a uni-directional data flow from parent components to child components via props and doesn't allow children access parent data. Vue, on the other hand, has $parent. I can probably get $parent working in the adapter, but it hasn't been very easy to do. The uni-directional data flow seems like a good idea in Vue too.

With all of this in mind, here are a couple of questions for you:

  1. How do you feel about avoiding things like style strings, domProps, and $parent in the Vue version? Would you be willing to change components already using these, or at least accept pull requests?

  2. How do you feel about this "adapter" approach? Any feedback or concerns?

  3. What are your thoughts about tests? It would be great to have at least some unit tests in both libraries to prevent regressions and ensure the libraries stay in sync. React has libraries like Enzyme to do unit tests that ensure that different combinations of props render the correct output. I don't see anything like that that supports Vue 2, but maybe you know of a library. Another option is to do Selenium tests with BrowserStack or Sauce Labs (I think both are free for open source projects). I have always found Selenium tests to be brittle and slow, but maybe a limited number of tests running on the kitchen sinks or something would be good.

  4. I saw you published docs for F7 Vue on the F7 website. Will you be adding the docs to GitHub so I can create docs for React? Are you open to adding the React docs to the F7 website once F7 React is stable and the docs are complete?

bencompton commented 7 years ago

UPDATE: I have now taken the automation a step further. As of my latest commit, the React components are now auto-generated in a new Gulp task (except for 3 that require custom React code). It still uses the reactifyF7Vue function I discussed in my last post, but now the files containing the code calling this function are generated automatically. It uses static and dynamic code analysis of the Vue components to determine the arguments to that function, like the tag name, events, slots, etc. For example, here is what the auto-generated Navbar component looks like:

import * as React from 'react'
import {reactifyF7Vue} from '../src/utils/reactifyF7Vue';
import {VueNavbar} from '../framework7-vue/framework7-vue';
import {NavLeft} from './NavLeft';
import {NavCenter} from './NavCenter';

export interface INavbarProps {
   backLink?: boolean | string;
   sliding?: boolean;
   title?: string;
   theme?: string;
   layout?: string;
   onNavbarBeforeinit?: (eventArgs?: any) => void;
   onNavbarInit?: (eventArgs?: any) => void;
   onNavbarReinit?: (eventArgs?: any) => void;
   onNavbarBeforeremove?: (eventArgs?: any) => void;
   beforeInnerSlot?: React.ReactElement<any>;
   afterInnerSlot?: React.ReactElement<any>;
   id?: string;
   className?: string;
   style?: string;
}

export const Navbar = reactifyF7Vue<INavbarProps>({
    component: VueNavbar,
    tag: 'f7-navbar',
    events: [
        'navbar:beforeinit',
        'navbar:init',
        'navbar:reinit',
        'navbar:beforeremove'
    ],
    instantiatedComponents: [
        NavLeft,
        NavCenter
    ],
    slots: [
        'before-inner',
        'after-inner'
    ]
});

What this means is that components can be added and modified in Framework7 Vue and they will be available in Framework7 React just by updating the Framework7 Vue version in the package.json. As of now, all of the components in F7 Vue 0.7.2 are available in this branch. However, only the first handful of kitchen sink pages have been ported over, so there is no guarantee that they all work yet. There are also still some bugs that need fixing in the "adaptor" (reactifyF7Vue) code, and of course there will need to be a few changes in the Vue version that I mentioned in my post above before all of the React components will work correctly.

This automation should save a significant amount of effort, but of course, it isn't perfect. The code analysis makes certain assumptions that may not be valid 100% of the time. Specifically, the tag names are obtained by analyzing src/framework7-vue.js, and if that file gets moved or refactored significantly, it may break the code analysis. In addition, in cases where $emit and $slots are accessed in a dynamic way, the React components won't have any props for the affected events / slots. For example, in the form-input component, the event name is passed to $emit in a variable. For cases like this, I have added the ability to specify overrides per component. So for the form-input component, the events are manually specified in the Gulp task, while everything else is still discovered automatically.

centrual commented 7 years ago

Great job @bencompton!

nolimits4web commented 7 years ago

@bencompton amazing!

  1. React does not accept strings on style attributes (to limit XSS attacks). For example, the Progressbar component is setting translate3d style attribute via a string, and it will not work in React. When the Vue code is modified so that the translate style attribute is specified in object instead of string format, it works just fine in React. I would think that specifying styles in object format would be a best practice in Vue as well because of XSS concerns.

Already moved everything to objects

  1. React strongly discourages using innerHTML as strings to prevent XSS attacks. It requires using the "dangerouslySetInnerHTML" prop in React. There are several Vue components that are using domProps.innerHTML. I currently have this kind of working (you'll notice a bug on the cards kitchen sink page where an element rendered like this is missing), but I would prefer not to support it.

Don't really like it too, but looks like there is no any workaround of this in Vue. But HTML-string props can save a lot of markup in such case. But will see, maybe, really, will drop support for such HTML-string props in F7 Vue as well

  1. React is pretty strict about having a uni-directional data flow from parent components to child components via props and doesn't allow children access parent data. Vue, on the other hand, has $parent. I can probably get $parent working in the adapter, but it hasn't been very easy to do. The uni-directional data flow seems like a good idea in Vue too.

Yeah, but, again, in some cases it helps to save a lot of extra markup. E.g. when we set media-list prop on List component, then ListItem should be rendered in a different way, now it is done via checking $parent media-list prop. Otherwise, each ListItem will require additional props

nolimits4web commented 7 years ago

@bencompton

  1. How do you feel about avoiding things like style strings, domProps, and $parent in the Vue version? Would you be willing to change components already using these, or at least accept pull requests?

Feel good and will accept pull requests :) . Just need to find a good workaround for those

  1. How do you feel about this "adapter" approach? Any feedback or concerns?

Looks very good and i like it, especially in terms of future support and keeping it in track with F7 Vue plugin.

  1. What are your thoughts about tests? It would be great to have at least some unit tests in both libraries to prevent regressions and ensure the libraries stay in sync. React has libraries like Enzyme to do unit tests that ensure that different combinations of props render the correct output. I don't see anything like that that supports Vue 2, but maybe you know of a library. Another option is to do Selenium tests with BrowserStack or Sauce Labs (I think both are free for open source projects). I have always found Selenium tests to be brittle and slow, but maybe a limited number of tests running on the kitchen sinks or something would be good.

Yeah, i think we need them. But i would like to dig deeper for tests a bit later, when we finalize the most of the things, so we can (probably) have same tests for both libs

  1. I saw you published docs for F7 Vue on the F7 website. Will you be adding the docs to GitHub so I can create docs for React? Are you open to adding the React docs to the F7 website once F7 React is stable and the docs are complete?

Framework7 website and all its docs already on GitHub in separate repo https://github.com/nolimits4web/Framework7-Website/tree/master/src/jade/vue. Of course, let's add and publish them as we get stable React plugin

bencompton commented 7 years ago

@nolimits4web

Already moved everything to objects

Thanks for updating the styles--working beautifully in Framework7 React now!

Don't really like it too, but looks like there is no any workaround of this in Vue. But HTML-string props can save a lot of markup in such case. But will see, maybe, really, will drop support for such HTML-string props in F7 Vue as well

How do you feel about using slots instead of using strings?

For example:

<f7-list media-list>
  <f7-list-item
    v-for="n in 2"
    :title="'Item ' + n"
    :subtitle="'Subtitle ' + n" 
    text="Some text goes here"
    after="After"
    link="http://google.com"
  >
    <img slot="media" :src="http://lorempixel.com/160/160/people/{n}" width="80" />
  </f7-list-item>
</f7-list>

In many cases, I suppose it will be necessary to determine whether or not a specific slot was passed in to decide whether or not to render the surrounding markup. That's easy in components that have a render method. For templates, maybe a v-if that checks a computed property that looks at $slots would work?

nolimits4web commented 7 years ago

@bencompton btw, how is the porting to React is going? Maybe I can help with something else?

bencompton commented 7 years ago

@nolimits4web - it is going very well and the plan is to release it on NPM next week.

All of the components are working, and an app I am developing has been upgraded to the new version. Aside from those few bug fixes I have contributed to Framework7 Vue recently, it has worked extremely well in my app.

Framework7 React is currently pointing to my own fork of Framework7 Vue to get those latest bug fixes, but once you release the latest F7 and F7 Vue on NPM, Framework7 React will be able to consume those and will then be able to be released. I saw you said you would be doing that in the next couple of days.

I am also working on docs for the React version and hoping to have that done for the release next week. It looks like I can auto-generate most of the docs from the Vue docs, which makes sense given that the components themselves are auto-generated from the Vue components. I'm pretty far into that and should have a PR for you to look at early next week.

The only other thing missing is starter templates, which I was hoping to get done next week too. Rather than making a Webpack and a Browserify template, I was considering just making a create-react-app template, since that seems to be the most popular option for React at the moment.

Incidentally, did you get a chance to look at #94? That is one last little design decision I have been mulling over. Ideally, the React and Vue versions would handle this the same way, and I would certainly be open to changing the React version to have F7 be separate if it turns out that makes more sense. Writing the installation docs got me thinking about this.

nolimits4web commented 7 years ago

@bencompton new versions are already released on npm, just need to update website docs with new APIs. As for #94, I agree to bundle F7 core with Vue/React plugins. But I think it will require a bunch of changes to make them correctly UMD'able. I will need to convert Swiper, F7, Dom7, Template7, Animate7 libs to such correct format