voorhoede / riotjs-style-guide

Opinionated RiotJS Style Guide for teams.
Creative Commons Zero v1.0 Universal
287 stars 22 forks source link

add "Sanatize tag option values" rule #36

Closed jbmoelker closed 8 years ago

jbmoelker commented 8 years ago

Summary

Proposed changes:

petergoes commented 8 years ago

I find the update / reduce example rather difficult. Especially when looking at the implementation example <output-sum numbers="{ [1, 2, 3, 4] }" /> <!-- outputs: 10 -->. Looking at this implementation, I assume that you put in a list (and expect that list to be preserved). However, that list ends up being a number. Which is quite the contrary to what the rule is actually dictating.

Can't we extend the first recommended block like:

this.items = opts.items || []; // default to empty list
this.score = Number(opts.score) || 0;  // default to 0
this.foo = this.foo || 'bar'; // default to 'bar'

I get what you want to explain in the example, but maybe we can change the naming from:

<output-sum total-score="{ [1, 2, 3, 4] }" /> <!-- outputs: 10 -->
petergoes commented 8 years ago

Ah... I completely misread the example (twice). I did not see the naming of the tag and the sum tag property.

But I still think the example is not clear. This is a rule about option values. For this example to make sense, you need to know that the element is outputting a single value.

phortuin commented 8 years ago

I think this rule has nothing to do with Riot, but with basic programming practices. I would scratch it entirely.

jbmoelker commented 8 years ago

@phortuin I'd say it's a generic software pattern (defensive programming) specifically applied to Riot. I've now seen issues in multiple projects which could have been avoided by using this pattern. Since to me the guide aims to help more robust Riot apps which are predictable in their use for other developers as well, I do think it's relevant.

Perhaps we can rephrase it to:

## Harness your tag options

In Riot your tag options are your API. A robust and predictable API makes your tags easy to use by other developers. Therefore you should **harness your tag options**.

...
How? --> defaults, type conversion, ...

@petergoes if we are going through with this PR (pending discussion with @phortuin) I agree that we can simplify the example. It was just one I still had lying around: http://plnkr.co/edit/RvPuBP?p=preview . I do think that the naming fits the component though as it "outputs the sum of the given numbers".

phortuin commented 8 years ago

That sounds reasonable, esp with the argument that the tag is your API. I would still keep it fairly generic, with a simple sanitization example for typing & fallback (something like const score = parseInt(opts.score, 10) and const fruits = opts.fruits || []). Even then, it's dangerous to explain too much before having to write a book on this (because what happens if opts.fruits is not an array in the before example, etc)

petergoes commented 8 years ago

I don't think there is anything wrong with the naming of the rule and its meaning. I found just the example a little confusing. I don't see the need to rephrasing it.

Maybe we can have another rule explaining the your-tag-is-your-api thing?

phortuin commented 8 years ago

Hm interesting, I wanted to state that explaining 'tag as api' is not necessary as it has become a common pattern thanks to Angular mostly, but when I google HTML as API and everything related, I get nothing. So I still think that it shouldn’t be part of this guide, but maybe we need to rephrase 'the tag is your api' and then write a blog post about this pattern and the cons/pros?

petergoes commented 8 years ago

I added a new question #37 so we can discuss the HTML as API thing.

How do we proceed with the Sanatize tag option values rule?

jbmoelker commented 8 years ago

@phortuin I also updated this PR based on feedback of you and @petergoes: https://github.com/voorhoede/riotjs-style-guide/pull/36/commits/00e0a9cfb2fdf57d853f29f3dc7ed9654284a5a6