peerlibrary / meteor-blaze-components

Reusable components for Blaze
http://components.meteorapp.com/
BSD 3-Clause "New" or "Revised" License
354 stars 26 forks source link

make sure data() always returns an object #98

Closed pixelass closed 9 years ago

pixelass commented 9 years ago

if using this.data() and no arguments are passed it returns undefined.

This often leads to issues as in can't read property foo of undefined

class Foo extends BlazeComponents {
  onCreated(){
    console.log(this.data().foo)
  }
}

works

{{>Foo foo="hello"}}
{{>Foo bar="hello"}}

does not work

{{>Foo}}

I am working with two different approaches to fix this issue

class Foo extends BlazeComponents {
  onCreated(){
    console.log(this.data() && this.data().foo)
  }
}

or

class Foo extends BlazeComponents {
  onCreated(){
    this.props = this.data() || {}
    console.log(this.props.foo)
  }
}

the second approach is from my newer components which adds an immutability layer (which makes sense for some of my components).
I still think an empty Object would be a better way to handle this issue

here's something I am currently trying

class ComponentBase extends BlazeComponent {

  /**
   * construct the class
   * public methods should be bound to `this`
   */

  constructor(){
    super();
  }

  // Template/Component methods
  onCreated(){
    // props should be immutable so do not attempt to
    // change the props at any time.
    // you can use the state which is a reactive object
    // See the `setState(obj)` method for info on state manipulation
    this.props = this.data && this.data() || {};
    this.state = new ReactiveVar({});
  }

  onRendered(){
    // rendering
  }

  onDestroyed(){
    // destroying
  }

  /**
   * allow to set the state of a component.
   * Simply call this method to set the current state of the component
   * You can pass nested objects.
   * All required updates are handled by this helper so you can be sure your component updates when needed
   * @param {Object} obj pass an object to extend the current state
   */
  setState(obj) {
    let state = this.state;
    for (let prop in obj) {
      state[prop] = obj[prop];
    }
    this.state.set(state);
  }

  // ... some methods

  events(){
    return [
      {
        //... some events
      }
    ];
  }
}

ComponentBase.register('ComponentBase');

not really sure if it makes sense but I am trying to make some things more like I am used to do do from React.js

mitar commented 9 years ago

So, this was a concuss design decision:

I like this approach because it can help in some cases to differentiate between those situations, but in most cases it behaves the same (there is not data context (null and undefined vs. there is some data context).

I know that data contexts are often objects, but it is not necessary so (looping over an array of strings). I know that some in community would like that they are always objects, but this is not how it is.

But the idea of Blaze Components is that they provide core functionality on top of which you can then build your abstractions. This is why I like that we do not hide too much information. But you can always hide that if you want by extending the base class:

class ComponentBase extends BlazeComponent {
  data() {
    return super.data() || {};
  }
}

BTW, here you will also convert data context false into {}.

For me personally this is not a problem. In CoffeeScript it is really easy to write @data()?.foo. :-)

mitar commented 9 years ago

BTW, look into reactive-field package. I think it is much better for use with Blaze Components than ReactiveVar.

Also, why are you passing props through data context? I would suggest that you pass that through component arguments. More or less that was what they were meant for (arguments which are the same for the whole life of the component).

{{>Foo args foo="hello"}}
class ComponentBase extends BlazeComponent {

  /**
   * construct the class
   * public methods should be bound to `this`
   */

  constructor(kwargs){
    super();

    // props should be immutable so do not attempt to
    // change the props at any time.
    // you can use the state which is a reactive object
    // See the `setState(obj)` method for info on state manipulation
    // TODO: Check if kwargs instanceof Spacebars.kw
    this.props = kwargs && kwargs.hash || {};
  }

  // Template/Component methods
  onCreated(){
    this.state = new ReactiveField({});
  }

  setState(obj) {
    let state = Tracker.nonreactive(() => this.state());
    for (let prop in obj) {
      state[prop] = obj[prop];
    }
    this.state(state);
  }

}
pixelass commented 9 years ago

coffescript is not an option for me. The reasons are probably obvious and don't need to be discussed.

anyways thanks for the explanation, at least I understand your concept now. I guess I will just go with my ComponentBase approach.

pixelass commented 9 years ago

sorry I missed the next comment..

let me read and answe on it

pixelass commented 9 years ago

I am using Jade (which I sadly realized is not really Jade)

with the Jade package I had problems using args and at some point I gave up and just used props.

I think my main problem is meteor in general. I am not a fan of meteor and for me it is very hard to adjust to their IMHO broken arcitechture. I have to use meteor at work though so I am trying to find ways to make development easier/more logical. your lib already helps a lot.

I will try out your suggestions and look into reactiveField.

I really appreciate your help. thx

mitar commented 9 years ago

Hm, I thought Jade should work exactly the same? But I never tried.

pixelass commented 9 years ago

It might again be the issue that the meteor-jade version is IMHO broken and I can't adjust to it. I am VERY good at Jade and even though the developer who made meteor-jade says:

Meteor-jade basically works like pure Jade

it is a lie.

e.g.

div hello

does not work because the dev chose to make it react the same way as

div(hello)

so you have to write

div
  | hello

I think the fact that spacebars allows functions without parentheses and the developer tried to use the same approach for meteor-jade it makes it very hard to understand what's going on, especially when you know a lot about Jade. My guess is that the developer of meteor-jade had no idea of the logic behind Jade and only tried to apply the lexer-logic to his package.

It is still nicer for me to write my components with the "jade-like" syntax though and just accept the fact that it's not jade.

I would never recommend the package to anyone.

Ending this discussion now though since it is already way off topic and I don't want to spam your issues.

If you ever want to know why I like Jade over Handlebars you can read the first few paragraphs of this blogpost I wrote a while ago: http://codepen.io/pixelass/post/let-us-jade-1

mitar commented 9 years ago

Hm, there is no other Jade Meteor package?

pixelass commented 9 years ago

sadly I only found forks of mquandalle:jade or packages that seem unfinished/deprecated or senseless

https://atmospherejs.com/mquandalle/jade?q=jade

From the jade-package

input($dyn = attrs)

seems to be the equivalent of

<input {{attrs}}>

I think I had issues using it in "components"

+myComponent($dyn = attrs)

while he is using the mixin decoration for components but does not allow real mixins

mixin foobar(foo)
  div=foo

would be a standard Jade feature but it will crash a meteor app.

mitar commented 9 years ago

Ah, yes, @mquandalle is using Jade as well.

pixelass commented 9 years ago

well that is the actual package I am using in my projects.

I just opened an issue requesting to remove the confusing statement that it is "basically like jade"