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

No way to override name attribute on e.g. <input>s to prevent overriding existing properties on `this` #715

Closed
dbkaplun opened this Issue May 12, 2015 · 14 comments

Comments

Projects
None yet
7 participants
@dbkaplun

dbkaplun commented May 12, 2015

No description provided.

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi May 12, 2015

Member

I am not sure this makes sense, if you are developing a tag, why would you have the need to name an input with an existing property's name? If you name an input 'update' currently it will crash. If riot stops it from being applied, you would be expecting this.update to be an input and not a function. Either way it would be best to avoid it. I put in a PR but not sure if it is beneficial.

Member

rsbondi commented May 12, 2015

I am not sure this makes sense, if you are developing a tag, why would you have the need to name an input with an existing property's name? If you name an input 'update' currently it will crash. If riot stops it from being applied, you would be expecting this.update to be an input and not a function. Either way it would be best to avoid it. I put in a PR but not sure if it is beneficial.

@dbkaplun

This comment has been minimized.

Show comment
Hide comment
@dbkaplun

dbkaplun May 12, 2015

I think it might be worthwhile to store the input elements into a new this.inputs or something along those lines. Semantically speaking one might not expect this[inputName] to be an element, rather an element's value.

dbkaplun commented May 12, 2015

I think it might be worthwhile to store the input elements into a new this.inputs or something along those lines. Semantically speaking one might not expect this[inputName] to be an element, rather an element's value.

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi May 12, 2015

Member

I think that is out of the scope of the intention. I am removing the PR.

Member

rsbondi commented May 12, 2015

I think that is out of the scope of the intention. I am removing the PR.

@GianlucaGuarini GianlucaGuarini added the bug label May 13, 2015

@GianlucaGuarini GianlucaGuarini referenced this issue May 23, 2015

Closed

Riot next #773

10 of 12 tasks complete
@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Jun 24, 2015

Member

I think we need some more opinions on this? What do you think guys?
Probably tag.inputs is not that bad

cc @tipiirai @cognitom @aMarCruz

Member

GianlucaGuarini commented Jun 24, 2015

I think we need some more opinions on this? What do you think guys?
Probably tag.inputs is not that bad

cc @tipiirai @cognitom @aMarCruz

@tipiirai

This comment has been minimized.

Show comment
Hide comment
@tipiirai

tipiirai Jun 25, 2015

Member

Initially I loved the shortcut syntax but wasn't sure anymore after facing the first name conflict. I'm thinking this.elem or this.dom for all named elements, not just inputs. Unfortunately we need a new major version for such massive breaking change.

Member

tipiirai commented Jun 25, 2015

Initially I loved the shortcut syntax but wasn't sure anymore after facing the first name conflict. I'm thinking this.elem or this.dom for all named elements, not just inputs. Unfortunately we need a new major version for such massive breaking change.

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Jun 25, 2015

Member

How about introducing this.elem on 2.x and removing the shortcut syntax on 3.0 ?

Or..., this.query() could be another option? I mean:

<my-tag>
  <form onsubmit={ submit }>
    <input name="comment" value={ value }>
  </form>

  submit(e) {
    var data = {
      comment: this.query('[name=comment]').value
    }
    doSomething(data)
  }
</my-tag>
Member

cognitom commented Jun 25, 2015

How about introducing this.elem on 2.x and removing the shortcut syntax on 3.0 ?

Or..., this.query() could be another option? I mean:

<my-tag>
  <form onsubmit={ submit }>
    <input name="comment" value={ value }>
  </form>

  submit(e) {
    var data = {
      comment: this.query('[name=comment]').value
    }
    doSomething(data)
  }
</my-tag>
@tipiirai

This comment has been minimized.

Show comment
Hide comment
@tipiirai

tipiirai Jun 26, 2015

Member

React uses ref: https://facebook.github.io/react/docs/more-about-refs.html

<input ref="myInput" />

this.refs.myInput

Maybe we mimic that directly. Helps React users in transition.

Member

tipiirai commented Jun 26, 2015

React uses ref: https://facebook.github.io/react/docs/more-about-refs.html

<input ref="myInput" />

this.refs.myInput

Maybe we mimic that directly. Helps React users in transition.

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Jun 26, 2015

Member

ref attribute looks natural :)

Member

cognitom commented Jun 26, 2015

ref attribute looks natural :)

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Jun 27, 2015

Member

hi all,
this and others issues are fixed in next PR of tmpl(), I don't know about tag().

And you're right, I wasted many time with class shortcuts, there is great inconsistency between shortcuts and normal expressions, and the code grows more than I expect. I think this needs breaking changes.

Before push, I would like to clarify a question.
Testing the new tmpl() I was confused with the results of this expressions:
// for tmpl('{ [yes,no].join("<br>") }', data)
// this function is generated:
// (assume data = { yes:true, no:false })

(function(d) {
    return [
        (function(v){try{v=("yes"in d?d:global).yes}finally{return v}}).call(d),
        (function(v){try{v=("no"in d?d:global).no}finally{return v}}).call(d)
    ].join("<br>");
})(data)
// returns ===> "true<br>false"

// for this: tmpl('<p foo="{ "x" }">{ [yes,no].join("<br>") }</p>', data)
// mixed templateText/expression, this is generated:

(function(d) {
    return [
        "<p foo=\"","x","\">",
        [
            (function(v){try{v=("yes"in d?d:global).yes}finally{return v||v===0?v:""}}).call(d),
            (function(v){try{v=("no"in d?d:global).no}finally{return v||v===0?v:""}}).call(d)
        ].join("<br>"),
        "</p>"].join("");
})(data)
// returns ===> '<p foo="x">true<br></p>'

I see this counterintuitive, the latter example must returns '<p foo="z">true<br>false</p>'
...but it seems right according with previous tmpl() behavior, based on riot specs.
Is the output ok?

Member

aMarCruz commented Jun 27, 2015

hi all,
this and others issues are fixed in next PR of tmpl(), I don't know about tag().

And you're right, I wasted many time with class shortcuts, there is great inconsistency between shortcuts and normal expressions, and the code grows more than I expect. I think this needs breaking changes.

Before push, I would like to clarify a question.
Testing the new tmpl() I was confused with the results of this expressions:
// for tmpl('{ [yes,no].join("<br>") }', data)
// this function is generated:
// (assume data = { yes:true, no:false })

(function(d) {
    return [
        (function(v){try{v=("yes"in d?d:global).yes}finally{return v}}).call(d),
        (function(v){try{v=("no"in d?d:global).no}finally{return v}}).call(d)
    ].join("<br>");
})(data)
// returns ===> "true<br>false"

// for this: tmpl('<p foo="{ "x" }">{ [yes,no].join("<br>") }</p>', data)
// mixed templateText/expression, this is generated:

(function(d) {
    return [
        "<p foo=\"","x","\">",
        [
            (function(v){try{v=("yes"in d?d:global).yes}finally{return v||v===0?v:""}}).call(d),
            (function(v){try{v=("no"in d?d:global).no}finally{return v||v===0?v:""}}).call(d)
        ].join("<br>"),
        "</p>"].join("");
})(data)
// returns ===> '<p foo="x">true<br></p>'

I see this counterintuitive, the latter example must returns '<p foo="z">true<br>false</p>'
...but it seems right according with previous tmpl() behavior, based on riot specs.
Is the output ok?

@samuelmesq

This comment has been minimized.

Show comment
Hide comment
@samuelmesq

samuelmesq Jul 2, 2015

Contributor

@cognitom this.query() is similar to document.querySelector(), by the way, is that i'm doing in my project right now this.root.querySelector(), works great for me.

Contributor

samuelmesq commented Jul 2, 2015

@cognitom this.query() is similar to document.querySelector(), by the way, is that i'm doing in my project right now this.root.querySelector(), works great for me.

@tipiirai

This comment has been minimized.

Show comment
Hide comment
@tipiirai

tipiirai Jul 4, 2015

Member

@aMarCruz not 100% sure what you are asking, but false should be rendered as an empty string. The only falsy value that should be rendered is 0 (number zero).

Member

tipiirai commented Jul 4, 2015

@aMarCruz not 100% sure what you are asking, but false should be rendered as an empty string. The only falsy value that should be rendered is 0 (number zero).

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Jul 4, 2015

Member

@tipiirai , I mean, the expression is the same in both examples: { [yes,no].join("<br>") }, the difference is the first is a (unique) expression, the latter is a template, and the result is not the same...
With class shorthands there may be differences in other cases, too.
So we're talking about three different behaviors with the same "expressions", just as is in the specs. right?
Perhaps I had a confusion with some terms :)
Thanks.

Member

aMarCruz commented Jul 4, 2015

@tipiirai , I mean, the expression is the same in both examples: { [yes,no].join("<br>") }, the difference is the first is a (unique) expression, the latter is a template, and the result is not the same...
With class shorthands there may be differences in other cases, too.
So we're talking about three different behaviors with the same "expressions", just as is in the specs. right?
Perhaps I had a confusion with some terms :)
Thanks.

@tipiirai

This comment has been minimized.

Show comment
Hide comment
@tipiirai

tipiirai Jul 5, 2015

Member

Oh. Gotcha.

I think the output is totally ok. Definitely not a blocker for the new tmpl implementation since we're talking about an edge case.

Member

tipiirai commented Jul 5, 2015

Oh. Gotcha.

I think the output is totally ok. Definitely not a blocker for the new tmpl implementation since we're talking about an edge case.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Oct 15, 2016

Member

this issue will be fixed in riot@3.0.0 using the ref property instead of name or ids

Member

GianlucaGuarini commented Oct 15, 2016

this issue will be fixed in riot@3.0.0 using the ref property instead of name or ids

@GianlucaGuarini GianlucaGuarini added this to the 3.0.0 milestone Oct 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment