Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#12281 closed defect (worksforme)

[patch][regression] dojo.Stateful wildcard handling broken

Reported by: rahul Owned by: Kris Zyp
Priority: high Milestone: 1.6
Component: Core Version: 1.6.0b1
Keywords: Cc:
Blocked By: Blocking:

Description

In r23763, names as recorded in dojo.Stateful book-keeping were changed with underscores being prepended to avoid clashes.

However, this now prepends underscores to even the name "*", which has wildcard semantics in dojo.Stateful, whereas the same consideration is not applied when looking up wildcard callbacks. As a result, those watches registered using wildcards will not receive any callbacks on property changes.

Attached is a patch that fixes this -- clearly there are multiple ways to fix, this patch retains the wilcard as "*" in dojo.Stateful book-keeping which suits my taste better than storing it as "_*".

Attachments (4)

stateful-fix-r23763.patch (576 bytes) - added by rahul 9 years ago.
Patch that fixes the wilcard related regression in dojo.Stateful.
12281-test-fail.patch (477 bytes) - added by rahul 9 years ago.
A failing test that passed in previous releases containing dojo.Stateful
doh-is-order.patch (1.1 KB) - added by rahul 9 years ago.
A patch to fix the order of arguments to the doh.is() calls in tests.Stateful.
ValidationTextBox-robot-watch-fix.patch (566 bytes) - added by rahul 9 years ago.
Fixes incorrect watch usage in dijit.form.ValidationTextBox? robot test.

Download all attachments as: .zip

Change History (16)

Changed 9 years ago by rahul

Attachment: stateful-fix-r23763.patch added

Patch that fixes the wilcard related regression in dojo.Stateful.

comment:1 Changed 9 years ago by bill

Milestone: tbd1.6
Owner: changed from anonymous to Kris Zyp

Isn't there a regression test for * handling? Is it failing?

comment:2 Changed 9 years ago by Kris Zyp

(In [23774]) Added wildcard test for Stateful, refs #12281

comment:3 Changed 9 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

I added a regression test, but it is passing. Wildcarding is done by omitting the first parameter to watch, which seems to still work properly.

comment:4 Changed 9 years ago by rahul

Resolution: fixed
Status: closedreopened

Reopening and I will provide a patch with a failing test that passes on prior releases. Background is that the MVC code I have [1] (more on that soon, BTW) broke when I recently updated it to the bleeding edge -- I consider that codebase to be a good regression test suite for all parts of dojo.Stateful :-)

That codebase uses the explicit wildcard watch pattern in a number of places i.e.

stateful.watch("*", function...

While I don't recollect the origin of the above usage (probably saw it somewhere else before using it), its the above usage variant that breaks post r23763 because the wildcard callbacks are now stored internally as property "_*". Applying stateful-fix-r23763.patch will cause the test (coming in a minute) to pass as it did in prior releases containing dojo.Stateful.

[1] http://doughays.dojotoolkit.org/dojomvc/

Changed 9 years ago by rahul

Attachment: 12281-test-fail.patch added

A failing test that passed in previous releases containing dojo.Stateful

comment:5 Changed 9 years ago by rahul

Oh, as an aside, I noted that in the dojo.Stateful doh tests the order of expected and actual values in the assertions is inverted i.e. should be doh.is(expected,actual) but the tests have doh.is(actual,expected) in all usages of doh.is() in the file. Happy to provide a patch separately if there is interest in correcting this.

comment:6 Changed 9 years ago by bill

Oh, it's a question of

stateful.watch("*", func)

vs

stateful.watch(func)

?

The former syntax worked in 1.5 and dijit is currently using it, in ValidationTextBox.html.

However, I checked the API doc for 1.5 and it says to use the latter syntax:

watch: function(/*String?*/name, /*Function*/callback){
// summary:
//		Watches a property for changes
//	name:
//		Indicates the property to watch. This is optional (the callback may be the 
// 		only parameter), and if omitted, all the properties will be watched

It doesn't mention anything about a * wildcard.

The reference documentation in 1.5 is merely a copy of the API doc, and thus says the same thing.

So either need to re-support passing in "*" as a name, or fix the calling code like ValidationTextBox.

comment:7 Changed 9 years ago by rahul

Correct, thats the question.

I think backwards compatibility should rule core Dojo APIs, unless things are outright buggy or its a major release with design changes (hopefully very few and far in between). While you can easily fix ValidationTextBox? and I can easily fix the MVC code, it will likely bite some other users as well. Lets spare them the trouble :-)

Changed 9 years ago by rahul

Attachment: doh-is-order.patch added

A patch to fix the order of arguments to the doh.is() calls in tests.Stateful.

comment:8 Changed 9 years ago by rahul

While I was in the vicinity, I whipped up a patch to fix the order of the doh.is() arguments. If applying all patches, suggest applying the doh.is one before the one that contains the failing test.

comment:9 Changed 9 years ago by Kris Zyp

(In [23778]) Reverse doh.is order patch from rahul, refs #12281

comment:10 Changed 9 years ago by Kris Zyp

Resolution: worksforme
Status: reopenedclosed

Fixed ValidationTextBox? in [23777].

Backwards compatibility does rule Dojo core APIs, and nothing has changed with the API. The API has always stated that you watch for all property changes by omitting the first property on the watch call and that still works properly. Using "*" was never part of the API, never supported, and shouldn't be used.

comment:11 Changed 9 years ago by rahul

A quick grep reveals one more instance of using "*" to watch all changes in the dijit.form.ValidationTextBox? robot test which should then be fixed as well (see forthcoming patch in a minute).

Changed 9 years ago by rahul

Fixes incorrect watch usage in dijit.form.ValidationTextBox? robot test.

comment:12 Changed 9 years ago by bill

That's already fixed from [23777] (above).

Note: See TracTickets for help on using tickets.