Closed GianlucaGuarini closed 7 years ago
What about removing the parent -> children properties inheritance? It seems to create only confusion, pollute the child tags and slow down the riot performances
Performance: Someone in gitter was just talking about a loop of items, which then looped inside a select box.
we have 250 adAccount, each adAccount need to display 50 consultant. 250 x 50 = 12500.
I'm guessing that each instances is also inheriting the entire parent, which is the problem? Not just the 12,500 instances but also duplicating the entire parent object 50 times.
Expect-ability:
Yes, having access to the parent prop without calling parent
is useful, but I was using message
and failure
variables (which may not be 'best practice') but they were inheriting from their parent! I was quite surprised and had to use childMessage
instead to get around this.
So I'm all for removing it... sure, we can wait for 3.0.0 for this possibly breaking change.
The one place you'll really want to keep the inheritance is for the "anonymous tags" created by each loops. Basically this should still work:
<span>{outer}</span>
<div each={item in list}>
<span class={active: item == outer}>{item}</span>
</div>
So we need to check if the tag looped is a custom one or an anonymous one.. it looks correct to me
I'm afraid of the exceptions and would rather cut out code.
But... it's namespaced here, into the item
variable, so the inheritance doesn't cause clobbering issues.
In terms of performance, is it a clone of the items, or a reference..?
I'm sure you guys have better instincts on what Riot needs, but FWIW, here are some things I had on my riot wishlist.
onclick={foo('bar')}
key
.each
(maybe using prototypes?)Allowing function arguments in callbacks
onclick={foo('bar')}
... that would be cool -- but not generally the way JS works. It would allow me to skip data-
values and event.target.getAttribute('data-)
calls. e.g.:
<span each={name,i in daysOfWeekHuman }>
<input type="button" data-day={i} value={name} onclick={seeDay}/>
</span>
this.daysOfWeekHuman = ['Sunday','Monday','Tuesday','Wednesday','Thursday','Friday','Saturday'];
this.seeDay = function (event){
var seeDay = event.target.getAttribute('data-day');
...
}
... with this:
<span each={name,i in daysOfWeekHuman }>
<input type="button" value={name} onclick={seeDay(i)}/>
</span>
this.daysOfWeekHuman = ['Sunday','Monday','Tuesday','Wednesday','Thursday','Friday','Saturday'];
this.seeDay = function (data){
var seeDay = data;
...
}
@avimar you should be able to do that with .bind.
onclick={ seeDay.bind(this, i) }
@rogueg great feedback thanks
For tests, define tags inline at the top of the test function. This makes them easier to read, write, and encourages shorter, more focused tests.
I agree I will clean this up as soon as the dev
branch will be merged into the next
branch
A way to avoid updating expensive parts of the page, similar to react's componentShouldUpdate
I thought about this and I guess we can use simply a flag isFrozen
, if it will be true we will freeze all the tag updates
Allowing function arguments in callbacks onclick={foo('bar')}
Here I would like to avoid overcomplicating the riot api, .bind(this, ...params)
seems to be the best option right now. Using data-val
seems to be also a valid alternative
Shortcut for 2-way binding of inputs.
Checking the result of this post I guess this feature is not necessary. I still prefer the one-way data flow.
When a critical opt changes, provide a way to completely reload that tag, similar to react's key.
Can you provide more info? From the riot doc it's not really clear to me what's really used for
Improve the performance of adding new tags and each (maybe using prototypes?)
Of course I agree
I have added all the feedback I like to the initial post. I hope to work again on the next
branch as soon as you guys will merge the dev
branch
My newer reorder snippet is faster and more flexible in most cases, you should definitely check that out: https://github.com/pakastin/f/blob/master/src/setchildren.js
Here's some benchmarks against others: http://pakastin.fi/perf
The reverse is the extreme case, because there's nothing to skip. When there's only few to reorder, or some of the views gets removed, it's even faster, because it can skip the actions cleverly, when needed.
I think another way to emulate componentShouldUpdate
could be returning false on an tag update handler like this:
this.on('update', function() {
// do some checking
return false; // if tag should not update
});
This would be possible if we had some changes on riot.observable
behavior as I proposed here https://github.com/riot/observable/issues/16
If you guys think it is a good idea, I can work on this.
@wellguimaraes I think it's a lot easier if we just detect the isFrozen
flag here and return
But probably I would set isFrozen
inside an update
handler, based on some checking, right?
Actually, the observable
change idea was not meant to solve the tag frozen state, but eventually it could be an alternative solution to prevent update, as it could be used to prevent unmount and so on...
What about removing the parent -> children properties inheritance? It seems to create only confusion, pollute the child tags and slow down the riot performances
+1, it created much confusion for me... accessing the parent opts is sometimes annoying, but more more predictable.
I like the idea of checking return values from update, but that could get ambiguous if there are multiple handlers attached. What about an explicit method?
shouldUpdate() {
if (opts.color == this.oldColor) return false
this.oldColor = opts.color
return true
}
If that method is present on the tag, then it gets run before an update. You could also gate parts of a tag, which is pretty cool:
<div shouldUpdate={colorChange}>
<span each={bigArray}>{color}</span>
</div>
colorChange() {
if(opts.color == this.oldColor) return false
this.oldColor = opts.color
return true
}
I could even imagine a helper method for checking when the value at some path (ie opts.color
) changes.
@rogueg I also like the idea of using a method to check whether the tag should update or not.
\ I believe it would not get that ambiguous with multiple update handlers. We just need to know that if any handler return false, the update process would stop.
@rogueg ok let's use shouldUpdate
I will update the next branch this weekend
Is the v3 alpha already usable ? is there a release date planed already ?
thanks for that great lib
@rvion try npm install riot@next
;)
@GianlucaGuarini thanks :)
(for now, I clone the next
branch and manually make min
)
minor remark: those 2 features :
What about removing the parent -> children properties inheritance? It seems to create only confusion, pollute the child tags and slow down the riot performances
and
Improve the performance of adding new tags and each (maybe using prototypes?)
from this thread do not appear in the top post summing up remaining items.
Hello @GianlucaGuarini, and thanks for this great framework. What is the current status of the "Experimental" Riot.Tag method used in manual tag construction. I really prefer this style of coding, because I don't like to mix logic with view in the same file.
A couple of suggestions for riot 3.0
At the moment if you have a custom component and add an 'onclick' attibute this gets interpreted as the whole component having this and gets handled by riot. ie
<cutomtag onclick="{somehandler}"></customtag>
There is no way to (that I know of) to handle this inside the component, ie just have a specific click area. This means we have to avoid native event handler attribute names. Custom attributes are much less 'guessable' than the native ones.
It would be really cool if we could..
<customtag>
<script>
this.onclick=function(e){
// this runs instead of native
}
</script>
</customtag>
this
This has bitten me a few times, when named inputs have clashed with variables in the tag. ie.
<customtag>
<input name="user" />
<script>
this.user = "fred"
this.on('update',function(){
console.log(this.user)//is this fred or the input domnode?
})
</script>
</customtag>
Removing this now would break a lot of code. But a warning in the compiler would be really useful, or some kind of runtime error. Possibly by setting the writable property of the attached references. I've not used writeable properties on objects but Object.defineProperties seems to be supported in IE9+.
It looks like putting a console.warn
in the objects setter could work, making a once use property
ie
obj = {}
Object.defineProperties(obj, {
"user": {
set:function(val){
if(!this.value){
this.value = val
}else{
console.warn("cannot set value of user")
}
}
}
})
obj.user = "hello" // this works
obj.user = "hello2" // this warns
@crisward
At the moment if you have a custom component and add an 'onclick' attibute this gets interpreted as the whole component having this and gets handled by riot.
If I've understood what you mean I guess it should be already possible http://plnkr.co/edit/foTx2Vect5KmcDe7BUmP?p=preview
names and id's being attached to this
This is a dangerous change because it will trigger warnings also if your properties will be changed manually without riot. I might think about this and probably we can simply cache the riot DOM nodes in a property like this.ref
. But this break change will be a pain for the whole riot ecosystem
@GianlucaGuarini
This is what I had in mind, http://plnkr.co/edit/lFKdf24ZbPX15BXtL8WM?p=preview
my-tag.html
So the internal this.onclick would override the tags onclick attribute. The default behaviour is to make the whole tag clickable which is sometimes useful to override.
tag2.html
It's also not possible to filter the event bubble as the outside one gets handled in parallel.
@crisward is this not enough http://plnkr.co/edit/GFEoRsDIHjmoD9NtbwF3?p=preview ?
@GianlucaGuarini it's still fires outside of the button, which means your component is unable to encapsulate it's own event logic if you use native event names. If the outside attribute was called on-click
or buttonpressed
it would work. But using native event names provides a much more predictable/familiar api. Not sure what other people think...
Are there any decision-making discussion threads or issues regarding this feature?
remove the automatic preventDefault from the riot DOM events
@MartinMuzatko I have updated the task description adding the issues/confusion of some of our users regarding this topic
@crisward why not this.root.addEventListener('click', function(e) {...})
?
@rsbondi thanks for the suggestions, it still fires the outside event. See example here
http://plnkr.co/edit/lFKdf24ZbPX15BXtL8WM?p=preview (my-tag.html)
You have both events attached to the same element, so there is no propagation to stop. This does not seem to be a riot issue, but maybe you can open a separate issue as we have strayed from the roadmap here
I realise that, that's why I'd like riot to check if a method on the tag already exists with the event name before adding one. So if this.onclick exists on the tag, don't add another onclick, instead use the one specified inside the component. I think it would be a useful addition for riot 3.0.
What spec for shouldUpdate ended up in the alpha? Is it component-wide or can it be used to gate parts of the tag as rogueg suggested?
@nodefish the shouldUpdate
method added to the riot alpha works on the whole component http://plnkr.co/edit/bZxn8Q0uEy0EB0RDV5WM?p=preview
@GianlucaGuarini Understood, thanks.
@GianlucaGuarini do you need help with the makefile? Do you already have plans to which build system to switch?
Webpack? Gulp? Something else?
Also, regarding removing preventDefault
and auto update
for DOM events. I'm sure this puts the user a lot more in control of what happens and what shouldn't. I'm sure it is confusing that sometimes you have to call update and sometimes not. Also that sometimes you have to preventDefault and sometimes not. So that change is perfectly valid. But I think it is a bit against our philosophy of unnecessary boilerplate. What do you think? More control vs ease of use and conventions? Because we had return true
earlier for enabling preventDefault (AFAIK)
Anyhow, I can't wait for the implementation details, once this version is released.
Rather than removing the feature, why not do what another framework does? They implement preventDefault by default and provide an option for you to enable it. The difference though is that they configure events using class properties, so they can pack different options in there.
I do like how Riot uses direct vanilla events, which is less coding, but how would you give the user the option to disable/enable preventDefault and stopPropagation? Maybe you can use a custom attribute?
On Monday, July 11, 2016, Martin Muzatko notifications@github.com wrote:
Also, regarding removing preventDefault and auto update for DOM events. I'm sure this puts the user a lot more in control of what happens and what shouldn't. I'm sure it is confusing that sometimes you have to call update and sometimes not. Also that sometimes you have to preventDefault and sometimes not. So that change is perfectly valid. But I think it is a bit against our philosophy of unnecessary boilerplate. What do you think? More control vs ease of use and conventions? Because we had return true earlier for enabling preventDefault (AFAIK)
Anyhow, I can't wait for the implementation details, once this version is released.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/riot/riot/issues/1694#issuecomment-231678150, or mute the thread https://github.com/notifications/unsubscribe/AAZkuffljAZ9PVp8F_QLSF-TVZXVo6ywks5qUgVogaJpZM4H0iWK .
@mykone a great plus of riot is, that you don't have to configure anything to make it work. There is a configuration object though. riot.settings
this is where this might belong. I'm sure it makes sense to have some conventions and standards, but maybe there is a reason not to add an option for that?
If this is a major gotcha for checkbox and radio options, then can you just add that preventDefault isn't called on those types? The general defaults seem to have worked rather well...
Martin. Yeah, riot.settings sounds like a good place, but that would affect all the events in that Riot view, but what if I want that behaviour for only one element in that view, and keep the others as default?
On Mon, Jul 11, 2016 at 8:03 AM, avimar notifications@github.com wrote:
If this is a major gotcha for checkbox and radio options, then can you just add that preventDefault isn't called on those types?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/riot/riot/issues/1694#issuecomment-231761652, or mute the thread https://github.com/notifications/unsubscribe/AAZkuWaL4HkN_CzWaWTrXH-e-IufFcEoks5qUlsqgaJpZM4H0iWK .
AFAIK the riot team wants to keep exceptions to a minimum. @avimar @mykone
Less cognitive load
@MartinMuzatko the new riot release will be focused on performances and stability rather than unpredictable behaviors (I don't like the magic stuff). The automatic prevent default and the auto DOM updates in the loops caused many problems for our users. The new riot simply will allow you to do event.preventDefault()
whenever you need it rather than doing it always per default sticking to the default javascript behavior. We have also upgraded our events handling using DOM 2 events listeners. Another big update will be the use of refs
instead of name
or id
to better scope the DOM elements queries under the same path this.refs.myDomNode
@cognitom should be already working on this feature
The magic stuff -- like child inheritance caused problems. Not just the unexpectedness, but even figuring out how to work around it.
Where will event.preventDefault()
be needed most? I think we'll need a robust upgrade guide. I think any onsubmit
handle will probably need that triggered.
Hmm. I wonder if there can be an upgrade-assist like lodash-migrate
? One that will log to console any time event.preventDefault()
was called automatically for you, as well as other things?
Another big update will be the use of refs instead of name or id to better scope the DOM elements queries under the same path this.refs.myDomNode
So until now, myTextBox was referenced by this.myTextBox for the DOM and this.myTextBox.value for the value. That did cause some confusion as I was.. hoping? for 2-way binding of this.myTextBox. But now at least they won't clobber each other. Is there any good way to link a dom node value directly to a variable for automatic 2-way updates?
I have no problem with having to call update()
after modifying the state. However the preventDefault()
I'm less convinced about. Many programmer won't know when to use preventDefault()
so will get unexpected consequences when creating client side applications, possibly introducing more common questions.
An obvious one for this is when using onsubmit
. Also anyone who has tried to write a drag feature knows how many events need preventDefault
adding.
When don't you want to preventDefault? I've checked my riot components and I don't think I've ever had to use the return true option. The debate between 'purity' and 'convenience' should fall in favour of easier to understand code, and if the browsers implement something in a difficult to understand ways I think frameworks like Riot are well within their remit to smooth those edges.
IMO it will be good to add migration switch like
riot.settings.preventDefault
, so old code will not be broken with new
Riot 3.0 behavior.
Regards, Dmytro.
On Tue, Jul 12, 2016 at 10:35 AM, Cris Ward notifications@github.com wrote:
I have no problem with having to call update() after modifying the state. However the preventDefault() I'm less convinced about. Many programmer won't know when to use preventDefault() so will get unexpected consequences when creating client side applications, possibly introducing more common questions.
An obvious one for this is when using onsubmit. Also anyone who has tried to write a drag feature knows how many events need preventDefault adding.
When don't you want to preventDefault? I've checked my riot components and I don't think I've ever had to use the return true option. The debate between 'purity' and 'convenience' should fall in favour of easier to understand code, and if the browsers implement something in a difficult to understand ways I think frameworks like Riot are well within their remit to smooth those edges.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/riot/riot/issues/1694#issuecomment-231961383, or mute the thread https://github.com/notifications/unsubscribe/ABtBWfLKHvp1x-0prGWaftGOB9Dp1UO-ks5qU0PdgaJpZM4H0iWK .
The magic stuff -- like child inheritance caused problems. Not just the unexpectedness, but even figuring out how to work around it.
Agreed. My main source of difficulty (not that Riot is difficult, overall) with Riot has been the magic. Another example is that Riot's shorthand functions automatically bind this
to the tag instance. It's expected for this
to be bound to the call context (e.g. scope from which the function is called), so in most cases there's no difference since you do call the functions from the Riot tag; however, in a few subtle and/or complex cases, it's quite a mess dealing with this, e.g. defining a function in a tag like so:
this.myFunc = function(){
// `this` doesn't refer to the real tag instance here, even though it looks like it does in the debugger!
}
(I'm not saying the above example should be changed, I just cited it for illustrative purposes of the dangers of magic.)
For me, explicit metaphors make for a much stronger design than magic. So, in general, I'm glad some of the more hazardous magical things are being toned down.
@crisward I feel strongly that preventDefault should be a deliberate decision by the programmer. If programmers don't know about it, then the Riot documentation should give a hint about what it means, as well as direct people to external docs that discuss preventDefault. This seems preferable to Riot hiding the decision from inexperienced programmers.
On a different note, to play the devil's advocate, maybe we should force the developer to make a decision on preventDefault? Meaning that Riot event handlers must return true or false, or throw a warning? Or if not a warning, then at least a style convention, i.e. in the docs/examples, all Riot event handlers have a return statement. This would completely remove the implicitness and preventDefault would be part of the thought process when writing Riot functions.
Having functions return a value isn't boilerplate, so I don't find it bad at all to return true/false in all of them.
Regarding preventDefault, I only needed to actually run the "default" once, which I just wrote last week:
<input type="text" onkeydown={handleEscape}>
to make a feature for ESC resets the box to the previous value.
And apparently that's only for the Escape button, since everything else is usually onkeyup/onchange which already had an action and wouldn't have needed to return true
So 99.9% of the time, I want preventDefault.
I'm actually concerned with the option of riot.settings.preventDefault
since that makes a huge difference across the codebase. Hmm, wouldn't that also affect e.g. including riot-gear's tags? The point is NOT to be backwards compatible -- that's what the increase in major version is for. The point is: "What's the best choice?" and I think the "most used" is the best one.
Regarding calling update()
I'm OK with that -- but it's still a big change -- since many of my changes happen async anyway I'm pretty sure I need to call it most of the time explicitly.
I am thinking about removing the spaced events in riot@3.0.0 to gain more speed it would be nice to have other opinions on this as well
3.0.0 will be quite a big release for riot. @rsbondi and the new member of the team @rogueg are already working on the
next
branch fixing many of the important issues that we can solve only by introducing breaking changes. Here you can check the list of the issues we want to close for the next major release:switch frommakefile
to a more crossplatform solution #2024ref
attribute instead of names and ids riot/1185import
also inside the tags compiler/69riot-route
from the core making it optional https://github.com/riot/riot/issues/1485if
conditions https://github.com/riot/riot/issues/1477 https://github.com/riot/riot/pull/1658update
and theupdated
events get triggered before any tag has been mounted https://github.com/riot/riot/issues/1661preventDefault
from the riot DOM events https://github.com/riot/riot/issues/1770 https://github.com/riot/riot/issues/1718 https://github.com/riot/riot/issues/526__
prefix for the "boolean" html attributes https://github.com/riot/riot/issues/276shouldUpdate
method to the tags to emulatecomponentShouldUpdate
in reactI also hope to have enough time to add a blog section on our site describing the riot status on any release.
I would like also to mention that we started a pledgie donation campaign and if you would like to sustain us and you like riot it might be nice to consider a donation at least to let us paying the costs of the riot maintenance (domain, saucelabs, browserstack.. )
:v: