ManuelDeLeon / viewmodel-react-explorer

MIT License
2 stars 1 forks source link

Make it valid with Inferno as well #1

Open antoninadert opened 6 years ago

antoninadert commented 6 years ago

As you now support Inferno with ViewModel, is this possible to update it to support Inferno as well ? You can see this project: https://github.com/mariusandra/pigeon-maps/blob/master/src/infact.js that uses infact as inferno+react to support both libraries out of the box.

ManuelDeLeon commented 6 years ago

The thing is that the Explorer is written in VM so it compiles to either one. I'd have to release (and maintain) another package.

With React going MIT and speeding up, I'm in the process of rewriting VM with performance in mind. It will not use Tracker as a dependency manager and it will only work with React .

I was thinking to switch to Inferno (mostly for the license) but now I see little point in doing so. It would be more work for almost no gain. React has more tools and a bigger ecosystem available to it.

On Oct 4, 2017 1:33 AM, "Antonin Adert" notifications@github.com wrote:

As you now support Inferno with ViewModel, is this possible to update it to support Inferno as well ? You can see this project: https://github.com/mariusandra/pigeon-maps/blob/ master/src/infact.js that uses infact as inferno+react to support both libraries out of the box.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ManuelDeLeon/viewmodel-react-explorer/issues/1, or mute the thread https://github.com/notifications/unsubscribe-auth/AED31ocBCdrbdEMXl4mDzJdiXZocYpvhks5sozSvgaJpZM4PtM8b .

antoninadert commented 6 years ago

I respect your choice and I believe it's smart because you have support from a big company like facebook, even if I still think Inferno is a better library because it is way smaller and way faster (try React 16, see this benchmarks ), with the same API. You can still use Inferno-compat aliases to use React stuff in Inferno, so the ecosystem is not that much of a problem. Will react ever be as fast? I doubt it, you can see in this article that rewrite makes it 'smarter not faster'

Well that's still a good news for React and I guess it is more stable than Inferno, so I understand your view. I am just afraid of being locked in React.

I am not feeling good using Facebook's software, because their stance has been awkward and an MIT license does not grant patents, but they have a lot of them so they might just sue you. If you want to focus on performance, then Inferno support is still a very good selling point, as for now and I guess for the years to come Inferno is still faster.

I believe interoperability between Inferno and React with VM is a must. It feels so good right now to be able to switch !

I understand the maintenance concerns you have though, and it's tough to support 2 codebases, that's why I suggested to do something like Infact. I will be sad if I can't use Inferno on your new VM but it actually depends on you, as I'm all-in with VM.

PS: As you don't use Tracker anymore, how do you plan to have reactivity with Meteor ? via an additional package ?

ManuelDeLeon commented 6 years ago

Adding Inferno support to VM isn't that big of a deal (as long as Inferno is compatible with React), so I'll add the option to the babel plugin. I won't add the support on the VM explorer because I'd then have to maintain two separate code bases. Not fun.

Will react ever be as fast? I think a better question is Is it fast enough?

As you don't use Tracker anymore, how do you plan to have reactivity with Meteor? I don't. The biggest reason VM is so slow is because of all the reruns Tracker does to achieve "eventual consistency". The options are leave VM as it is or "break it" a little bit in order to gain better performance. I'm opting for the latter.

Reactivity in Meteor will work the same way it does for other reactive sources like Firebase and RethinkDB. You have to somehow observe changes in a collection and update the component accordingly. In the case of Meteor you can use collection .observe/.observeChanges or put the collection-data-fetch inside an autorun. In both cases you still have update the component accordingly.

So instead of doing something like

people() { return PeopleCollection.find().fetch(); }

You'd do:

people: [],
created() {
  Tracker.autorun(() => {
    this.people( PeopleCollection.find().fetch() );
  })
}

Not as nice, but something's got to give in the name of performance. The alternative is leaving VM as it is.

antoninadert commented 6 years ago

Maintenance opportunities Maybe I'll give it a try and maintain the VM explorer for Inferno myself. If you know what to change I would be happy to know since it would take a lot of time for me to understand the full code alone, but I guess once it is done it will not be too hard to maintain for me, along with inferno-proto-starter.

Fast enough ? But every day we want to be faster, that's the problem about performance. People want to support crappy devices as the internet will come to smaller devices (IoT, watches...) in the future, that's why we never have enough performance.

Performance with benchmarks ? Regarding your thoughts, do you only tackle performance issues through reactivity ? Do you plan to actually test it in benchmarks ? (I believe it doesn't make sense to have performance goals without being able to measure it). For me it would make a lot of sense if you were to have a little demo project that would render stuff with React /Redux /Proptypes and another to do it with VM, and that could compare the differences in rendering time, and allow users to play with some interactive animation stuff with a lot of data, to actually perceive the difference.

Breaking change ? Instead of your suggestion for breaking changes , why not having a binding reactive:true that would enable the reactivity ? So if I just want the old syntax I can do <Component b="reactive:true" /> that would allow to keep writing people() { return PeopleCollection.find().fetch(); } in my component

I believe there should be another VM structure, that is missing today, and that would be able to use properties defined in VM globals, share and mixins. This structure would help to have a migration path by defining a default behavior for some or all components.

The big idea is to declare in a single place, for each component, what we expect as default bindings, so we don't have to declare it in every component again.

Something using CSS selectors like that:

ViewModel.defaultBindings({
    Components: [reactive:true], //Valid for all VM & React components,
    HTML: [class:default], //Valid for all HTML components (div, a, button...),
    Ads: [reactive:true] //the key can be a component (or default HTML component)
    Home>a:  [click:NavigationClick],  //All the <a> inside the <Home /> component
    div#myFirstDiv:  [defer:true, click:EaseOutAnimation], //selectable by ID, this would apply in all my VM and HTML that match the selector.
    button.primary: [click:SubmitClick], //selectable by class
    }
})

Of course if the user declares the same binding in a specific component with another value, this will override the defaultGlobalBinding.

For me it would make a lot of sense, since I want to be able to fine-tune what is reactive and what is not. I love your simple reactivity out-of-the-box (one of the big plus of VM if you ask me), but it's true it's not always necessary, and it's not the default behavior I want.

Other advantage I see for this structure is to avoid what I have today to enable Single-Page-Navigation in my inferno-proto-starter:

      <a href="/about" b="click:NavigationClick">About Us</a>, 
      <a href="/ideas" b="click:NavigationClick">Ideas</a>, 
      <a href="/ads" b="click:NavigationClick">Ads</a>, 
      <a href="/" b="click:NavigationClick">Home</a>

It feels better to have this instead (< Home /> component):

      <a href="/about">About Us</a>, 
      <a href="/ideas">Ideas</a>, 
      <a href="/ads" >Ads</a>, 
      <a href="/">Home</a>

and also this (Store.js):

ViewModel.defaultBindings({
    Home>a:  [click:NavigationClick]
})

ViewModel.global({
   NavigationClick: function (event) {
        if (Meteor.isClient) {
            event.preventDefault(); // prevent full page reload
            const module = require('/Hub/client/hubClient');
            const History = module.History
            History.push(event.currentTarget.getAttribute('href')); // do SPA navigation
        }
    }
})

What do you think ?

ManuelDeLeon commented 6 years ago

Maintenance opportunities One of my goals for this iteration is to make it easier for people to contribute. For example, right now the test suite I use is so clunky that I'm the only one who can run it (it's mix of old Meteor testing procedures). I'm now rewriting all tests to run on Jest and I'm loving it. I'll put a sign on all repos welcoming any and all contributions. You could certainly maintain the VM-Explorer for Inferno.

Fast enough Not much of a point since you'd be maintaining the Inferno side, but I take the Rails approach of being happy that it's good enough for 99% of the apps out there.

Performance Right now I'm profiling the crap of every line I write. I just rewrote the code that resolves bindings and I'm in the process of profiling that.

Breaking Change We would then have to create yet another package to support Meteor. I don't think it's worth it since the alternative (wrapping the call in an autorun) isn't onerous.

defaultBindings

I think what you want are events. I'm going to revisit the topic because I don't remember why I didn't include it in the React version.

antoninadert commented 6 years ago

maintenance opportunities Thank you for this. When it's ready, please tell me if you'd prefer to write the repo and I maintain it, or if you prefer me to host it on my profile on github. In the case where I would need to create the first version myself, I would like some more docs or a quick chat with you for knowledge transfer.

Fast enough I agree, if it does the job then all good. I still believe that you cannot be faster by adding more convoluted things. Inferno's approach of keeping it minimal is what I feel comfortable with. It reflects very well on benchmarsk (Inferno is the closest thing to vanilla optimized JS performances)

Breaking Change If you actually perform this change like you intend to, it would make sense to either document a migration path or to release it in a new repo and let the old one for legacy, what do you think? This is a breaking change, some people may not be willing to follow. BTW, if you republish something, it may be a good opportunity for rebranding (I guess you want to attract more people in VM) As you prepare to get rid of Meteor's built-in integration and provide connection for more platform / DBs, I believe you should write integration sections about Meteor, RethinkDB (and Firebase, Apollo?) whatever you think is mainstream, should have an example of (real-time) integration in the docs.

defaultBindings I didn't know about events, but yes it seems like that pretty much. If I define Events in my Viewmodel.global it should do the trick. Can you have class and ID selectors in events ? (I see that you can do 'click button' so component selector is supported, but I don't know this syntax)

ManuelDeLeon commented 6 years ago

Yes, there should be a migration section in the documentation. With events you should be able to find elements with any CSS selector.

As for breaking changes, I don't like them any more than the next person but the change isn't so dramatic, the only thing I can think of is that it won't be tightly coupled with Meteor collections. But it will definitely work with them, as it currently can with other realtime/reactive things like Firebase.