Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#11537 closed enhancement (fixed)

Document and implement Dojo Store API

Reported by: Kris Zyp Owned by: Kris Zyp
Priority: high Milestone: 1.6
Component: Data Version: 1.5
Keywords: Cc:
Blocked By: Blocking:

Description

This ticket is to track the efforts of implementing the Dojo Object Store API, based on the W3C IndexedDB's object store API, and intended to provide a next generation, simplified successor to Dojo Data and Dojo Storage. This is based on discussions on the dojo-dev mailing list: http://thread.gmane.org/gmane.comp.web.dojo.devel/12314/focus=12316 And the wiki page: http://docs.dojocampus.org/dojo/store

Attachments (2)

data_model.patch (15.9 KB) - added by Jonathan Bond-Caron 9 years ago.
Use data model instead of getIdentity()
store-doc-whitespace-23674.patch (20.4 KB) - added by Kenneth G. Franqueiro 8 years ago.
Patch to fix remaining issues in doc whitespace as of after [23674]

Download all attachments as: .zip

Change History (103)

comment:1 Changed 9 years ago by Kris Zyp

(In [22679]) Initial commit of dojo.store.JsonRest?, a RESTful implementation of Dojo object store, refs #11537

comment:2 Changed 9 years ago by Kris Zyp

(In [22680]) Added ObjectStore? adapter for new object store to dojo data, refs #11537

comment:3 Changed 9 years ago by Ben Lowery

(In [22688]) Merged revisions 22670,22679-22681 via svnmerge from https://svn.dojotoolkit.org/src/dojo/trunk

Had to merge a conflict in _base/_loader/bootstrap.js. Seemed harmless, just a whitespace / var consolidation change.

........

r22670 | jburke | 2010-08-04 00:03:14 -0400 (Wed, 04 Aug 2010) | 1 line

Refs #11510: fix issue with regexp looking like a comment for debugAtAllCosts removal.

........

r22679 | kzyp | 2010-08-05 17:21:51 -0400 (Thu, 05 Aug 2010) | 1 line

Initial commit of dojo.store.JsonRest?, a RESTful implementation of Dojo object store, refs #11537

........

r22680 | kzyp | 2010-08-05 17:35:29 -0400 (Thu, 05 Aug 2010) | 2 lines

Added ObjectStore? adapter for new object store to dojo data, refs #11537

........

r22681 | dante | 2010-08-05 18:15:33 -0400 (Thu, 05 Aug 2010) | 1 line

fixes #11511 - deprecate djConfig attribute and global. use dojoConfig global or data-dojo-config attribute. updates tests, adds regression test for djConfig \!strict

........

comment:4 Changed 9 years ago by Kris Zyp

(In [22744]) Read total count from content-range and switch from using totalCount to total, refs #11537

comment:5 Changed 9 years ago by Kris Zyp

(In [22849]) Added QueryResults? wrapper for convenience functions, used and tested in JsonRest?, refs #11537

comment:6 Changed 9 years ago by Kris Zyp

(In [22850]) Added Dojo Data adapter for using Data Data store through the object store API, refs #11537

comment:7 Changed 9 years ago by Kris Zyp

(In [22851]) Added memory-based object store, refs #11537

comment:8 Changed 9 years ago by Eugene Lazutkin

Please consider using "remove" instead of "delete". The latter will be a problem to use until all browsers, engine, and IDEs implement "reserved words as properties". At least provide an alias.

comment:9 Changed 9 years ago by John Hann

Hey Kris, it looks like dojo.store.DataStore?'s get() returns an object as described in the wiki page (http://docs.dojocampus.org/dojo/store), but objects returned from dojo.store.DataStore?'s query() have additional "private" properties (such as _RI, _S, and _0 from an ItemFileWriteStore?). This is because get() recreates the object by referencing store.getAttributes(), but query() does not. Therefore, the "ensure ease of enumeration of data properties" principle in wiki is violated. (This also messes up any attempts at building general-purpose routines around the objects.) Any plans to fix this?

Also, I was expecting you'd be decorating returned objects with dojo.Stateful (to also help conform to the features described in the wiki). It's not clear to me how I would extend dojo.store.DataStore? to do that easily and efficiently. I guess a developer could do that with an additional dfd in the subclass's query method? I also see that he could do it by injecting his own onComplete function on the options parameter...

Both of those solutions seem harder than they need to be. Any suggestions? Or am I just being obtuse? :)

I also expected a subscribe method on the returned query results (like the wiki suggests). That's a critical feature for any writeable store. (But that looks easy to add in a subclass.)

FWIW, I'm not just asking hypothetically. I was almost done writing this same module, but yours is better and I'd like to rely on it in cujo.js once 1.6 is out. The get/set and watch methods on the objects -- as well as the subscribe method on the result sets -- are critical to cujo.js's data binding.

Love the new API, btw. Thanks for your help!

-- J

comment:10 Changed 9 years ago by Kris Zyp

(In [22863]) Switched from "delete" to "remove" for deleting objects Fixed item to object conversion on DataStore? result sets Separated out the query engine, which can be replaced, and added sorting and paging capability refs #11537

comment:11 Changed 9 years ago by Kris Zyp

(In [22864]) Added Watchable module for adding notification/watch support on query result sets, refs #11537

comment:12 Changed 9 years ago by Kris Zyp

(In [22865]) Fix item to object conversion on get, refs #11537

comment:13 in reply to:  9 Changed 9 years ago by Kris Zyp

Replying to unscriptable:

Hey Kris, it looks like dojo.store.DataStore?'s get() returns an object as described in the wiki page (http://docs.dojocampus.org/dojo/store), but objects returned from dojo.store.DataStore?'s query() have additional "private" properties (such as _RI, _S, and _0 from an ItemFileWriteStore?). This is because get() recreates the object by referencing store.getAttributes(), but query() does not. Therefore, the "ensure ease of enumeration of data properties" principle in wiki is violated. (This also messes up any attempts at building general-purpose routines around the objects.) Any plans to fix this?

This should be fixed now.

Also, I was expecting you'd be decorating returned objects with dojo.Stateful (to also help conform to the features described in the wiki). It's not clear to me how I would extend dojo.store.DataStore? to do that easily and efficiently. I guess a developer could do that with an additional dfd in the subclass's query method? I also see that he could do it by injecting his own onComplete function on the options parameter...

I am hesitant to require the get/set/watch on all returned objects, as many times they won't be necessary, it adds to the burden of creating stores and slows things down, and it is pretty easy to add these functions to the objects returned from a store when it is needed:

stateful = dojo.Stateful(object); stateful.watch(...); stateful.set(...);

I also expected a subscribe method on the returned query results (like the wiki suggests). That's a critical feature for any writeable store. (But that looks easy to add in a subclass.)

I checked in the Watchable store wrapper that I have been working on. This provides the change notification capabilities. You can check out the Watchable tests to see how it is used.

FWIW, I'm not just asking hypothetically. I was almost done writing this same module, but yours is better and I'd like to rely on it in cujo.js once 1.6 is out. The get/set and watch methods on the objects -- as well as the subscribe method on the result sets -- are critical to cujo.js's data binding.

Let me know if you see any other issues, or have problems with these approaches. Thanks, Kris

comment:14 Changed 9 years ago by John Hann

Hey Kris,

Looks great! Still grokking it. But I noticed this:

Shouldn't line 22 of dojo.store.Watchable test for queryExecutor? If a data store doesn't have a queryExecutor, then the if(changed){} code path would fail because queryExecutor would be undefined.

It should be

if(query && queryExecutor){

or

if(queryExecutor){

Right?

-- John

comment:15 Changed 9 years ago by Kris Zyp

(In [22866]) Test for queryExecutor properly, refs #11537

comment:16 Changed 9 years ago by John Hann

Hey Kris,

If the result set returned from dojo.store.Watchable could return an unwatch function to the watch method (like Stateful.watch does) that would be awesome. :)

-- J

comment:17 Changed 9 years ago by Kris Zyp

(In [22867]) Added unwatch method to object returned from watch method, refs #11537

comment:18 Changed 9 years ago by John Hann

Woot! Successfully using the following for data binding on three hierarchical levels of nested views/widgets!!! (all done using declarative data binding, btw)

var store = dojo.store.Watchable(new dojo.store.DataStore?(new ItemFileWriteStore?({identifier:' id', items: someData})));

Only a few things that aren't clear and/or working as I'd expect:

1) dojo.store.DataStore? is sniffing this.idProperty = this.store.getIdentityAttributes() too soon since the identity attributes aren't defined (in ItemFileXXXStore at least) until a fetchItemByIdentity() or fetch() are executed. I am working around this, but it seems that get() ad query() might be better places to put the sniff?

2) The callback generated within results.watch() always deletes a matched data item even if the callback is fired in response to a put() that didn't change the data sufficient to remove it from -- or move it within -- the result set. It's critical (in my project at least) that we be able to preserve the views that correspond with each data item. They're expensive to manufacture. Furthermore, the thrashing of heap space (combined with poor GC in the older browsers) causes the RAM usage to go up faster than you'd think.

It's easy to just say that this is a View/Widget? concern, not a data store concern. I could certainly devise a setTimeout-based work-around for detecting when subsequent delete/insert calls are really a move (or a no-op). However, I'd think that this would also be a problem for grids and other heavy widgets that are now correctly listening to events on result sets instead of data stores. Thoughts?

3) Unless I am reading this wrong (since I didn't test it), if more than one listener executes the results.watch(), then multiple callbacks are created. This would be cool except that each callback performs the delete and/or insert into the result set. If there are three calls to watch(), then the deletes/inserts get performed three times.

Conversely, if nothing ever calls results.watch(), then the data items are never removed/inserted! I am doubtful that this is actually a problem, since if any outside logic is interested in changes, they should be using watch(). It just sure feels like Schroedinger's Cat. :)

Gonna try a server-side (async) store next.

-- J

comment:19 in reply to:  18 Changed 9 years ago by Kris Zyp

Replying to unscriptable:

1) dojo.store.DataStore? is sniffing this.idProperty = this.store.getIdentityAttributes() too soon since the identity attributes aren't defined (in ItemFileXXXStore at least) until a fetchItemByIdentity() or fetch() are executed. I am working around this, but it seems that get() ad query() might be better places to put the sniff?

Yeah, I noticed that too, when I was creating tests. That's a good idea moving to a point where it is actually needed.

2) The callback generated within results.watch() always deletes a matched data item even if the callback is fired in response to a put() that didn't change the data sufficient to remove it from -- or move it within -- the result set. It's critical (in my project at least) that we be able to preserve the views that correspond with each data item. They're expensive to manufacture. Furthermore, the thrashing of heap space (combined with poor GC in the older browsers) causes the RAM usage to go up faster than you'd think.

It's easy to just say that this is a View/Widget? concern, not a data store concern. I could certainly devise a setTimeout-based work-around for detecting when subsequent delete/insert calls are really a move (or a no-op). However, I'd think that this would also be a problem for grids and other heavy widgets that are now correctly listening to events on result sets instead of data stores. Thoughts?

Suppressing the event, or using a single event for an update that does not affect the index or inclusion of the object does seem like a good idea. Using a single event for an update that results in a move to a different index position seems trickier. If you have a suggestion for an alternate API, let me know. However, the current API is nice in that it follows the same callback argument signature as dojo.Stateful and https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/watch. But with this API, the callback only takes a single property id (the array index value), so I am not sure how you could denote the source and target of the move.

Also, I would think that views would typically want to use a setTimeout pattern for updates even in if single events were used for moves, since multiple moves might have been fired off in sequence by some multi-object action (I know the grid does this by default).

3) Unless I am reading this wrong (since I didn't test it), if more than one listener executes the results.watch(), then multiple callbacks are created. This would be cool except that each callback performs the delete and/or insert into the result set. If there are three calls to watch(), then the deletes/inserts get performed three times.

Conversely, if nothing ever calls results.watch(), then the data items are never removed/inserted! I am doubtful that this is actually a problem, since if any outside logic is interested in changes, they should be using watch(). It just sure feels like Schroedinger's Cat. :)

Good catch, Thanks.

comment:20 Changed 9 years ago by Kris Zyp

(In [22873]) Properly handle multiple watch listeners (without incurring GC problems), refs #11537

comment:21 Changed 9 years ago by Kris Zyp

(In [22874]) Directly query getIdentityAttributes if idProperty is not set, refs #11537

comment:22 Changed 9 years ago by John Hann

Also, I would think that views would typically want to use a setTimeout pattern for updates even in if single events were used for moves, since multiple moves might have been fired off in sequence by some multi-object action (I know the grid does this by default).

Yah, a setTimeout is probably in order at some point in the design. Still, it feels like we should suppress meaningless / no-op events.

If you have a suggestion for an alternate API, let me know. However, the current API is nice in that it follows the same callback argument signature as dojo.Stateful...

Heh, you're kinda stretching the id / oldValue / newValue parameter signature, imho. :) This seems more in the spirit of the native watch to me:

function watch (itemOrId, oldIndex, newIndex) {}

In the case of a deletion, itemOrId could be an id (primitive). For an add/move, itemOrId should [probably] be an object.

If newIndex is missing, then we've removed the item. If oldIndex is missing, we've added it. If they're both present, it was moved. If they're both missing, we didn't know what to do with it (no queryEngine).

I'm probably missing some use case since this seems way too simple. :)

comment:23 in reply to:  22 Changed 9 years ago by Kris Zyp

Replying to unscriptable:

Heh, you're kinda stretching the id / oldValue / newValue parameter signature, imho. :) This seems more in the spirit of the native watch to me:

function watch (itemOrId, oldIndex, newIndex) {}

In the case of a deletion, itemOrId could be an id (primitive). For an add/move, itemOrId should [probably] be an object.

If newIndex is missing, then we've removed the item. If oldIndex is missing, we've added it. If they're both present, it was moved. If they're both missing, we didn't know what to do with it (no queryEngine).

This seems doable, but I am not sure how it follows the standard watch signature. The index isn't a property of the object, it is a property of the array that holds the object (the array we are watching). Also, this is a little tricky to use since the first param could be an id or an object. But, If we did that, we would probably be better off not using the name "watch" (maybe go back to "subscribe").

I'm probably missing some use case since this seems way too simple. :)

I don't think so, it is a good idea.

comment:24 Changed 9 years ago by John Hann

OK. I am with you about 'subscribe' versus 'watch'. We're talking about mutations to the result set, not property changes.

We could remove the unnecessary parameters, then:

results.subscribe('insert', function(item, newIndex){ })

results.subscribe('remove', function(id, oldIndex){ })

results.subscribe('move', function(item, oldIndex, newIndex){ })

results.subscribe('indeterminate', function(item){ /* punt. let the dev figure out if the item is inserted or moved */ })

results.subscribe('*', function(itemOrId, oldIndex, newIndex){ /* catch-all. use param sniffing */ })

This looks a lot like your original suggestion on the wiki, iirc (with a few suggestions). :)

I specifically avoided using the word 'update' or 'onUpdate' since those terms are typically used to signify property changes. We want to emphasize that we're NOT listening to property changes in the data items at this level.

If you told me you weren't going to implement 'move' at all, I could live with that. I'd just implement a setTimeout-based solution like we discussed above. Let me know one way or the other.

Same goes for the '*' event. I just thought I'd throw that out there.

comment:25 Changed 9 years ago by bill

Wait, how do you listen to "property changes", a.k.a. updates, to items in the result set? http://docs.dojocampus.org/dojo/store?action=recall&rev=32 documents an onUpdate() call but Kris removed it a few days ago (http://docs.dojocampus.org/dojo/store?action=diff&rev1=32&rev2=33#line-127)

We talked about this in http://thread.gmane.org/gmane.comp.web.dojo.devel/13058/focus=13169, I hoped we had agreed not to use object.watch() to reflect updates to objects in the store.

PS: I like the idea of the explicit "move" notification, too bad it can't be used for Tree when an item is reparented (ex: a mail message is dragged from one folder to another).

comment:26 Changed 9 years ago by John Hann

Hey Bill,

Before I heard that the data stores were being revamped, I had originally coded my own data models with the ability to listen to item property changes by watching the entire result set. For my purposes at least, this ended up being more code and we weren't able to identify any uses cases (that couldn't be coded more efficiently another way).

My vote would be to keep the result set events as result set mutations, i.e. not to include item property changes in the mix.

Fwiw, I imagine it wouldn't be too hard for somebody to code a 'parent' mutation event notification to handle hierarchical moves as in Trees.

Now that I'm thinking more about it, maybe 'move' is the wrong word. 'index' (or 'position' or something similar) might be a better one.

comment:27 Changed 9 years ago by bill

Well, the use case for Tree is that each TreeNode needs to respond to changes in the name or icon of the corresponding object.

comment:28 Changed 9 years ago by John Hann

So you're opposed to item.watch()?

comment:29 Changed 9 years ago by Kris Zyp

My vote would be to include item property changes in events from the result set. I also don't think it is really that helpful to break the events into separate named events. From what I have seen, virtually every use case of notifications is to get a result set, render it, and respond to any changes in it, I have never seen a real-world case where you want to respond to removes, but not inserts, or updates.

Bill, is it problematic for reparenting to be denoted by an insert into one set of children, and a removal from another set?

comment:30 Changed 9 years ago by John Hann

Afaik, Nicole Sullivan coined the phrase "Separate container and content" when talking about OOCSS. However, it's also an important MVC concept when trying to identify and create reusable UI components. In summary, Container components manage layout and placement. Content components manage content (you coulda guessed that).

Several of the dojo widgets don't follow this separation. They manage the container and the content in a single drop-in component. I guess it makes sense for these components to want to listen for property changes in the result set, but the general MVC rule would be to listen for properties on a data item or to listen for collection mutations on a result set.

Ideally, the result set's methods to watch property changes and collection mutations would be separate. If you included property changes in the result set events, how would you indicate that the item's data changed and not it's position in the current API?

Kris wrote:

I also don't think it is really that helpful to break the events into separate named events... I have never seen a real-world case where you want to respond to removes, but not inserts, or updates.

Good call. If you're interested in inserts, then you're interested in deletes (and, by extension, moves). But if we're not going to implement the 'move' event at the data store level (again, I am ok with this), then we're only talking about 2 events to hook up. It's likely that if we combined those two into one event handler, we'd only be branching into two logic paths anyways. I don't see the benefit of combining them.

What if we used results.watch() to listen for data item property changes, but used a different mechanism to listen for collection mutation changes?

results.watch(propChangedCallback); property changes

results.subscribe(addCallback, deleteCallback); if these callbacks were combined, I could live with that. :)

comment:31 Changed 9 years ago by John Hann

Fwiw, if we're going to watch for properties at the result set level, why not allow JSONPath (in the future)?

results.watch(cb) default ==> results.watch('$[*].*', cb);

results.watch('$[*].patient.status', patientStatusChanged);

function patientStatusChanged (/* JSONPath */ propPath, oldVal, newVal) {}

This would degrade nicely to conform to the native watch() method for flat data structures.

comment:32 Changed 9 years ago by Kris Zyp

That's a good point John. I guess my primary concern is with forcing something like the grid to have to watch every single object that is rendered, which can add up to a lot watch listeners. With the subscribe approach, updates that do not affect the order or set of objects in a result set would appear as notification events where the old and new index value is identical. Would it be onerous for listeners that aren't interested in updates to check for index value equivalence? Or I guess we could have a parameter for subscribe to indicate whether or not to include updates.

comment:33 Changed 9 years ago by John Hann

After talking to the other guy on my team (Brian), we're cool if we have to specify an optional parameter to a subscribe/watch method in order to only receive mutation events.

comment:34 Changed 9 years ago by Kris Zyp

Anybody want to vote on watch(function(index,existingId,newObject)) vs subscribe(function(object,oldIndex,newIndex))? And if the latter, any naming preferences between "subscribe", "monitor", or "observe"? Observable seems the easiest resultant conjugation for the class that provides this capability.

comment:35 Changed 9 years ago by John Hann

+1 for observe

comment:36 Changed 9 years ago by bill

Hi guys,

So you're opposed to item.watch()?

item.watch() should correspond with item.set(). However, especially for a client-server store like JSONRestStore, item.set("foo", "bar") doesn't update the item in the data store, but rather just modifies a local copy of the item. Therefore (to be symmetrical) item.watch() shouldn't report on updates to the items in the data store, but instead should report local changes to items that may/may not be written back to the data store in the future.

Bill, is it problematic for reparenting to be denoted by an insert into one set of children, and a removal from another set?

Yes, but no more problematic than the current Tree code (running against the current dojo.data), which gets two notifications every time an item is reparented, and it needs to deduce that the item was moved rather than an one item being deleted and another being created.

comment:37 Changed 9 years ago by Kris Zyp

(In [22877]) Watchable renamed to Observable, fixing multiple listener errors, changed signature for single event moves, refs #11537

comment:38 Changed 9 years ago by John Hann

Hey Kris,

Couple o' things when deleting and adding when using dojo.store.Observable and dojo.store.DataStore? on an ItemFileWriteStore?.

var store = dojo.store.Observable(new dojo.store.DataStore({store: myItemFileWriteStore}));
var query = store.query({});

// in my code, the remove happens much later, but this should simulate the issue:
dojo.when(query, function (results) {
    store.remove(1);
});

This yields the following error since store.getIdentity is not defined on DataStore?:

dojo.store.Observable line ~34: TypeError?: store.getIdentity is not a function

If I define a getIdentity method, the following error happens:

dojo.store.Observable line ~37: TypeError?: results.splice is not a function

Line ~47 seems to be in the same category: results.push isn't a function, either.

-- J

comment:39 Changed 9 years ago by Kris Zyp

(In [22893]) Handle asynchronous results in Observable Added getIdentity to DataStore? refs #11537

comment:40 Changed 9 years ago by John Hann

Hey Kris,

This works awesome as long as you're deleting the last item in the resultSet! :D

unhappy_codez == mutating + iterating;

// remove the old one
for(i = 0, l = resultsArray.length; i < l; i++){
    var object = resultsArray[i];
    if(store.getIdentity(object) == existingId){
        removedObject = object;
        removedFrom = i;
        resultsArray.splice(i, 1);
    }
}

Btw, would it be a good idea to stop iterating once we've removed the deleted item? Or are we accommodating resultSets in which items may not be uniquely identified by store.getIdentity(item)?

-- J

comment:41 Changed 9 years ago by John Hann

I am not sure if this is a problem, but DataStore?'s remove method does not return a promise. As you know, Observable's "private" whenFinished function uses dojo.when() on the store's remove() to determine when to notify listeners that the item has been deleted. I guess this could be a problem if the item being deleted hasn't been fetched (or is no longer cached). ???

Like I said, I am not sure if this is a problem since I didn't study it very thoroughly.

comment:42 Changed 9 years ago by Kris Zyp

(In [22904]) Remove unnecessary dojo.declares, refs #11537

comment:43 Changed 9 years ago by Kris Zyp

(In [22905]) Fix object removal from result set, and observe any changes from put or add calls, refs #11537

Changed 9 years ago by Jonathan Bond-Caron

Attachment: data_model.patch added

Use data model instead of getIdentity()

comment:44 Changed 9 years ago by Jonathan Bond-Caron

Small note my patch,

dojo.data.Model should probably be renamed to dojo.data.ItemModel?

I didn't the test patch, just proof of concept / alternative approach.

comment:45 Changed 9 years ago by Jonathan Bond-Caron

I'm a little confused about the 'Returned Objects'.

Shouldn't there be way to "force" the returned object to not be modified in any way.

Say I'm doing store.get("invoice.5666") http://www.w3.org/TR/IndexedDB/

Ideally I should have away to get the same "plain" js object consistently across all data stores.

i.e.

store.get("invoice.5666", {plain: true}); returns object as-in from storage

storeDojo.get("invoice.5666"); returns object with get(), set() methods etc.. storeDojo.get("invoice.5666", {plain: true}); returns object as-in from storage

comment:46 Changed 9 years ago by Kris Zyp

(In [22968]) Correctly include the total count from query results, refs #11537

comment:47 Changed 9 years ago by Kris Zyp

(In [22973]) Improves placement of modified objects, refs #11537

comment:48 Changed 9 years ago by Kris Zyp

(In [22993]) Improves placement of modified objects, refs #11537

comment:49 Changed 9 years ago by Kris Zyp

(In [23209]) Added add, getIdentity to JsonRest?, including etag headers, added store tests to core tests, refs #11537 !strict

comment:50 Changed 8 years ago by Kris Zyp

(In [23298]) Use result delegation in case it is a frozen promise, refs #11537

comment:51 Changed 8 years ago by Kris Zyp

(In [23304]) Fixed sorting logic, fixed data tracking in memory, added unit tests, added Cache store wrapper, refs #11537

comment:52 Changed 8 years ago by Kris Zyp

(In [23305]) Remove unnecessary caching options, simplifying query logic, refs #11537

comment:53 Changed 8 years ago by Kris Zyp

(In [23328]) Only delegate on promises, refs #11537

comment:54 Changed 8 years ago by Kris Zyp

(In [23384]) objectProvider -> objectStore, refs #11537

comment:55 Changed 8 years ago by Kris Zyp

(In [23425]) Use delegate instead of mixin, a little lighter, refs #11537

comment:56 Changed 8 years ago by Kris Zyp

(In [23426]) Throw error on non-existent named filter function, refs #11537

comment:57 Changed 8 years ago by Kris Zyp

(In [23427]) Better type handling, refs #11537

comment:58 Changed 8 years ago by Kris Zyp

(In [23431]) Correct English on error message, refs #11537

comment:59 Changed 8 years ago by ben hockey

kris,

should put return the id? i noticed that Memory store doesn't return anything from put - not sure about other stores.

comment:60 Changed 8 years ago by Kris Zyp

(In [23481]) Copy listeners and queryUpdates for robust iteration, replace dismiss with cancel, and better handling of indexes without queryExecutor, refs #11537

comment:61 Changed 8 years ago by Kris Zyp

(In [23482]) Return an id from Memory's put and add methods, refs #11537 !strict

comment:62 Changed 8 years ago by Kris Zyp

(In [23573]) Handle no query, shorter code, refs #11537

comment:63 Changed 8 years ago by Kris Zyp

(In [23574]) Reference dependency modules to save some bytes, refs #11537 !strict

comment:64 in reply to:  63 Changed 8 years ago by ben hockey

Replying to kzyp:

(In [23574]) Reference dependency modules to save some bytes, refs #11537 !strict

while this will probably work if you build with requirejs, i believe this will break with the current state of dojo's build system. i haven't tried it in this specific case but i believe this is one of the limitations i've found due to the removal of the factory. once we have a complete amd build tool then it would be possible.

comment:65 Changed 8 years ago by Kris Zyp

(In [23576]) Undo reference dependency modules to save some bytes, refs #11537 !strict

comment:66 Changed 8 years ago by Kris Zyp

(In [23649]) Improved documentation, fixes order of caching operations, refs #11537

comment:67 Changed 8 years ago by Tom Trenka

(In [23656]) Example documentation for the Cache and some additional keyword argument objects for the simple query engine. Refs #11537 !strict.

comment:68 Changed 8 years ago by Tom Trenka

(In [23657]) More example documentation for the Cache. Refs #11537 !strict.

comment:69 Changed 8 years ago by Kris Zyp

(In [23670]) Return QueryResults? from map and filter, refs #11537

comment:70 Changed 8 years ago by Tom Trenka

(In [23674]) Finish up documentation for dojo.store. Refs #11537 !strict

Changed 8 years ago by Kenneth G. Franqueiro

Patch to fix remaining issues in doc whitespace as of after [23674]

comment:71 Changed 8 years ago by Kenneth G. Franqueiro

(In [23678]) Improving consistency of indentation in doc comments. Refs #11537

comment:72 Changed 8 years ago by bill

Observable.testQuery is failing on IE8 (and maybe other versions of IE too). Works on FF and Chrome. No meaningful error message about the failure.

comment:73 Changed 8 years ago by Kris Zyp

(In [23721]) Fixed indexOf call for IE compat, #refs #11537

comment:74 Changed 8 years ago by Kris Zyp

(In [23726]) Added support for regular expressions in SimpleQueryEngine?.js, and added wildcard -> regexp transformation in ObjectStore?.js for backwards compatibility with wildcard based queries, refs #11537

comment:75 Changed 8 years ago by Kris Zyp

(In [23748]) Don't remove object if no queryExecutor and object is being updated, refs #11537

comment:76 Changed 8 years ago by ben hockey

(In [23749]) fix typo in r23726. refs #11537

comment:77 Changed 8 years ago by Kris Zyp

(In [23750]) Don't remove object if no queryExecutor and object is being updated, but still use correct removedFrom index, refs #11537

comment:78 Changed 8 years ago by Kris Zyp

(In [23751]) Use shorter parseInt, refs #11537

comment:79 Changed 8 years ago by Kris Zyp

(In [23752]) Fix abort(), getLabel() handling, and avoid query modification, refs #11537

comment:80 Changed 8 years ago by Kris Zyp

(In [23753]) Look for ignoreCase on queryOptions properly, refs #11537

comment:81 Changed 8 years ago by Kris Zyp

(In [23755]) Go through store.notify on notifications, use -1 as missing item, look for queryEngine in cachingStore, add Cache test, refs #11537

comment:82 Changed 8 years ago by Kris Zyp

(In [23761]) Switch to dojo.declare refs #11537 !strict

comment:83 in reply to:  82 Changed 8 years ago by ben hockey

Replying to kzyp:

(In [23761]) Switch to dojo.declare refs #11537 !strict

you might need to try running some of these changes with dojo's build tool. i believe that it's going to break. i think you need to have a format something like

define([...], function(...) {
    dojo.declare('dojo.foo', ...);

    return dojo.foo;
});

having return dojo.declare(...); is going to break after the build tool does it's transform and removes the module's factory.

i haven't checked these changes specifically but that's just based on my experience so far. it's painful to keep in mind that this is not *really* AMD and so you have to write your code to match the pattern that works. dojo.Stateful is a good example of what should work.

comment:84 Changed 8 years ago by Kris Zyp

(In [23765]) Return class properly for build, refs #11537 !strict

comment:85 Changed 8 years ago by Kris Zyp

(In [23792]) Added tests and correction for ObjectStore?.newItem refs #11537

comment:86 Changed 8 years ago by Kris Zyp

(In [23793]) Added onNew event to ObjectStore?, refs #11537

comment:87 Changed 8 years ago by Kris Zyp

(In [23797]) Indicate notifications are supported, refs #11537

comment:88 Changed 8 years ago by liucougar

in [23755], tests/store.js include dojo.tests.store.JsonRest?, which requires dojo.xhrGet

dojo.xhrGet is only available in browser (not in rhino), so I think it should be guarded with dojo.isBrowser

(this breaks rhino tests)

comment:89 Changed 8 years ago by Kenneth G. Franqueiro

FWIW the dojo.store page on the docs wiki looks like it could use some cleanup - all the links on methods are dead, there's an unanswered question mid-page (which I presume we should just remove), and dojo.store.Cache is indicated as "future" when in fact it exists now.

I'm happy to do the job of removing the dead links, question, and the word "future" if that's indeed the approved course of action.

comment:90 Changed 8 years ago by Kris Zyp

Approved! Thank you!

comment:91 Changed 8 years ago by Kenneth G. Franqueiro

I've counted two unprotected for...in loops, one at DataStore.js:68, another at util/SimpleQueryEngine.js:82, which could be problematic if anyone tries to use these in a site with any naughty scripts that add to Object.prototype. To fix or not to fix?

comment:92 Changed 8 years ago by Kris Zyp

I actively seek ways to punish anyone who uses Object.prototype augmentation. And I certainly don't want to punish those who don't (with slower performance). Don't fix.

comment:93 Changed 8 years ago by Kenneth G. Franqueiro

Someone on IRC tonight was having trouble adapting their custom object store with dojo.data.ObjectStore. When I looked, I noticed that the implementation of fetch seems to assume that the store's query implementation supports RegExp.

This is not at all documented, and I'm not sure it's generally a requirement (would it even work with dojo.store.JsonRest?). Is there something we can do to mitigate this in time for release?

comment:94 Changed 8 years ago by Kris Zyp

(In [23904]) Return original wildcard string in toString for the sake of non-regex enabled stores (JsonRest?), refs #11537

comment:95 Changed 8 years ago by liucougar

(in [23906]) fixes #12354: updated rhino hostenv support to properly work with AMD formatted dojo sources

refs #11537: tests.store.JsonRest? should not run in non-browser environment

Deferred test needs a default timeout (a number), otherwise rhino complains about NaN error when setTimeout is called

comment:96 Changed 8 years ago by Kris Zyp

(In [23944]) Added store to prototype for declarative instantiation, refs #11537

comment:97 Changed 8 years ago by Tom Trenka

(In [23973]) Minor modifications to the ObjectStore?, refs #11537

comment:98 Changed 8 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

comment:99 Changed 8 years ago by Kris Zyp

(In [24239]) backport fixes from refs #12491, refs #12597, and refs #11537

comment:100 Changed 8 years ago by bill

In [26520]:

dojo.store and !data.data.ObjectStore? tests should be part of dojo test suite, refs #11537.

comment:101 Changed 7 years ago by bill

In [28327]:

fix typo in comment, refs #11537

Note: See TracTickets for help on using tickets.