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

[Prototype] Backbone Sub Vue #5308

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[Prototype] Backbone Sub Vue #5308

wants to merge 5 commits into from

Conversation

jmif
Copy link
Contributor

@jmif jmif commented May 8, 2019

This idea came out of a discussion with @AndrewJakubowicz on how we can being reskinning the level UX. During the reskinning we do not want to reimplement existing Backbone views (we want to leverage the existing logic) but we do want to be able to write the new skins in Vue. Ideally we could use Vue as a replacement for the template engine for these existing Backbone views until we have time to the rest of the view.

This fits well with our smart / dumb component model. If we treat the existing Backbone view as a "smart" component (it has the business logic already implemented) then our Vue work for this project can exist entirely as "dumb" views. If we implment the reskin as a set of Vue components that only take props (as presentational components should) then we can simply use those components again when we later migrate the smart Backbone view to Vue.

It turns our we're already using this model in places within our code base. LevelGoalsView.coffee is a great example, it uses Backbone to orchestrate the view and then passes the data to a rendered Vue component. It's implementation doesn't quite fit with the presentational component structure we'd like (it does not use props, making the component incompatible with smart components).

This PR adds a prototype SubVueComponentView that is based off of VueComponentView. It standardizes the rendering of a set of presentational Vue components within a Backbone view and allows data to be shared from Backbone to Vue via a simple setState method.

An example integration was shown with LevelGoalsView.coffee. To integrate:

  1. Extend SubVueComponentView instead of CocoView
  2. Set VueComponent as an instance variable pointing to the Vue component you want to render.
  3. Call setState whenever you have new data you want to pass to the Vue component.

The rest is handled for you.

We can also apply this to root views. Using the same model we can use VueComponentView to render a presentational Vue component in place of a Jade template. The steps are the same as above except we subclass VueComponentView instead of SubVueComponentView.

This can also be modified to support events and backbone mediator if necessary. With minimal work we can proxy events between the two.


const silentStore = { commit: _.noop, dispatch: _.noop }

export default class VueComponentView extends CocoView {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix name

@@ -10,14 +9,16 @@ const silentStore = { commit: _.noop, dispatch: _.noop }
module.exports = class VueComponentView extends RootView {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VueComponnetView and SubVueCompnentView are close to identical in terms of functionality with the exception that they inherit from different base classes and thus render differently. I don't see a way to avoid having two of them but couldn't think of an easy way to share functionality outside of a mixin and didn't want to spend the time doing that for a prototype.

If we move forward with this we should discuss ways to consolidate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that both these are identical and could possibly be consolidated. I would like to understand the rationale behind inheriting from CocoView instead of RootView for SubVueCompnentView

super.afterRender()
}

destroy() {
destroy () {
super.destroy()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we should add this on master if this PR doesn't eventually get improved / merged into master.

super.destroy()

this.vueComponent.$destroy()
this.vueComponent.$store = silentStore
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure how much $destroy does. Would we also want to do a this.vueComponent = null?

This is a great backbone interface with Vue.

return new this.VueComponent({
el: this.getMountPoint(),
propsData: this.props,
store: store
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to optionally be able to register a functional store? Could this be passed in via the constructor?

throw new Error('Class must be initialized with VueComponent')
}

this.props = {}
Copy link
Contributor

@shubhi1092 shubhi1092 May 9, 2019

Choose a reason for hiding this comment

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

Should we set some initial values to the state here in the constructor, just like in VueComponentView

propsData: { showStatus: true }
})

@setState({ showStatus: true })
Copy link
Contributor

@shubhi1092 shubhi1092 May 9, 2019

Choose a reason for hiding this comment

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

Related to my constructor comment above, we could set the initial state using the constructor, and then use @setState for changing it afterward

type: Boolean
default: false

overallStatus:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any downside of moving all the state from data to props? From what I know, the vue component should not mutate props and use only the data for mutating values/state. In such a case, would it be a good idea if the Vue component wants to change the state?

Copy link
Contributor

@codeluggage codeluggage left a comment

Choose a reason for hiding this comment

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

This looks awesome, and like a great tool for bridging from where we are to a future of Vue 🎉

I'll echo Andrew's comment on the store, which would make this even more usable.

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

Successfully merging this pull request may close these issues.

None yet

4 participants