Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16006 closed defect (invalid)

JsonRestStore / dojox.calendar / IE8

Reported by: Pete Smith Owned by: Kris Zyp
Priority: undecided Milestone: tbd
Component: Data Version: 1.8.0
Keywords: Cc: dg, cjolif
Blocked By: Blocking:

Description

I know, yuck. Possibly this is due to a misuse of the new promise architecture.

var store = new JsonRestStore?({target : 'test.json' }); var results = store.query({}); results = results.map(function(item){

return item;

});

Is this the correct way to map the results of a promise? Others are talking about this here: https://github.com/damiengarbarino/dojo-calendar/issues/15

The bottom line is, if you take the dojox/calendar/tests/calendar.html and try it in IE8, (using the MemoryStore?) all is fine. If I replace that with a dojo/store/JsonRestStore that gets the right data, it works great all over EXCEPT IE8. Maybe the restStore needs to be updated with the new dojo/request/xhr?

Attachments (2)

calendar.html (13.9 KB) - added by Pete Smith 7 years ago.
The original dojox/test page that has been rigged up against my jsonRestStore.
StoreMixin.patch (1008 bytes) - added by Kitson Kelly 7 years ago.
potential "fix" for issue

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by Pete Smith

Attachment: calendar.html added

The original dojox/test page that has been rigged up against my jsonRestStore.

comment:1 Changed 7 years ago by cjolif

Cc: dg cjolif added

comment:2 Changed 7 years ago by Pete Smith

Gents, if you can please contact me, my company is willing to invest in a solution to this problem, fast. 781-771-9234. I can work with you to show you how to set this up, do remote control, whatever it takes.

comment:3 Changed 7 years ago by Kenneth G. Franqueiro

After discussion on IRC, Kitson discovered that the improper use of map in question is actually within dojox calendar. Yes, this is an improper use of map, because the *new* results will *no longer* be wrapped with QueryResults?.

This issue seems to lie squarely on dojox calendar and not on dojo store, so I would think a new github issue should be opened there (since the old one was closed by the OP even before Christophe responded).

Changed 7 years ago by Kitson Kelly

Attachment: StoreMixin.patch added

potential "fix" for issue

comment:4 Changed 7 years ago by Kitson Kelly

The main issue is that the return results are a promise that when it gets resolved becomes the results (plus the fulfilled promise). Either results.then or when(results) should be used before attempting to process the results.

In most cases, the promise is being fulfilled prior to the results being accessed, but it is like dodging a bullet 9/10 times.

It is also questionable using ES5 array functions (like map) and expecting it to always behave properly on non-ES5 browsers. This module requires in dojo/_lang/array and should use that for all array functions.

There is also trailing whitespace in several places. Not very good coding style.

comment:5 Changed 7 years ago by Kenneth G. Franqueiro

Except that at the time results.map is called, results should be an object wrapped with dojo/store/util/QueryResults which guarantees presence of a map method. I'm actually not convinced that's the issue in itself. What's far more mysterious is that httpete sees no errors reported whatsoever.

comment:6 in reply to:  4 Changed 7 years ago by cjolif

Replying to kitsonk:

The main issue is that the return results are a promise that when it gets resolved becomes the results (plus the fulfilled promise). Either results.then or when(results) should be used before attempting to process the results.

In most cases, the promise is being fulfilled prior to the results being accessed, but it is like dodging a bullet 9/10 times.

It is also questionable using ES5 array functions (like map) and expecting it to always behave properly on non-ES5 browsers. This module requires in dojo/_lang/array and should use that for all array functions.

No as kgf is saying the result must be a QueryResults and you can iterate with map on it. And if you look into the code of QueryResults you will see that the map method is doing the when() call. So I don't think you correctly spotted the issue.

Maybe (but I'm not sure) the issue is more than we have two when here. The underlying when() from map and the one we are explicitly doing. And maybe there is an ordering issue. I will try to provide a patch in that direction to the user and if that solves is problem we will be good.

There is also trailing whitespace in several places. Not very good coding style.

Well... if that was the only coding style issue in dojox I would be more than happy...

comment:7 Changed 7 years ago by cjolif

Hmmm... actually the second when() is called on the QueryResults returned by the first one (map) not on the original one. So there is not ordering issue. It should work. If anyone has a clue...

comment:8 Changed 7 years ago by ben hockey

in order to try and understand this issue, i setup a jsfiddle that tries to isolate the JsonRest? code to see if it is responsible for this issue - http://jsfiddle.net/neonstalwart/XLhGm/ is the URL and the code is included below for convenience

        var store = new JsonRest({ 
                // get some data from https://gist.github.com/557b8d72ea444cd62e84
                target: '/gh/gist/response.json/557b8d72ea444cd62e84/'
            }),
            results = store.query(),
            mapped = results.map(function (item, i) {
                return 'index: ' + i;
            });
        
        when(mapped, function (m) {
            console.log('mapped results', m);
        });
        
        when(results, function (r) {
            console.log('raw results', r);
        });

i can't get this to fail in IE8. is there something more i need to do to produce the failure? i'd like to find the minimal amount of code needed to make this fail.

comment:9 Changed 7 years ago by ben hockey

the issue appears to be related to Date. IE8 does not understand new Date(anISOString). in the Memory example, the dates are not created like that but using JRS they are - that's the difference. if we switch the Memory example to use ISO strings for the data that gets passed to the store, you should see the issue.

here's the diff to apply to dojox/calendar/tests/calendar.html to do that:

  • dojox/calendar/tests/calendar.html

     
    2323
    2424                        require(["dojo/ready", "dojo/_base/lang", "dojo/_base/sniff", "dojo/_base/array", "dojo/_base/fx", "dojo/on",
    2525                                 "dojo/date/locale", "dojo/parser",     "dojo/dom", "dojo/dom-construct",       "dojo/store/Memory",
    26                                  "dojo/store/Observable",       "dojox/calendar/Calendar", "dijit/Calendar",  "dijit/TitlePane",
     26                                 "dojo/store/Observable",       "dojox/calendar/Calendar", "dojo/date/stamp", "dijit/Calendar",  "dijit/TitlePane",
    2727                                 "dijit/layout/BorderContainer", "dijit/layout/ContentPane", "dijit/form/CheckBox",
    2828                                 "dijit/form/TextBox", "dijit/form/DateTextBox", "dijit/form/TimeTextBox",
    2929                                 "dijit/form/Button", "dijit/form/ComboBox", "dijit/Menu", "dijit/MenuItem"     ],
    3030
    3131                                function(ready, lang, has, arr, fx, on, locale, parser, dom, domConstruct,
    32                                         Memory, Observable, Calendar){
     32                                        Memory, Observable, Calendar, stamp){
    3333
    3434                                        ready(function(){
    3535                                       
     
    8888
    8989                                                        newObj.endTime = calendar.dateModule.add(newObj.startTime, "minute", modelBase[id].duration);
    9090
     91                                                        // use ISO strings to match the data coming from a remote store
     92                                                        newObj.startTime = stamp.toISOString(newObj.startTime);
     93                                                        newObj.endTime = stamp.toISOString(newObj.endTime);
     94
    9195                                                        someData.push(newObj);
    9296                                                }
Last edited 7 years ago by ben hockey (previous) (diff)

comment:10 Changed 7 years ago by Pete Smith

The wizards at Sitepen found that the calendar was not correctly built, relying on native Date() methods of decoding isoStrings to dates. IE8 does not have this. This function on my dojox/calendar/Calendar fixes it.

provide a way to decode a date that is compatible with IE8

calendar.decodeDate = function (date) {

return stamp.fromISOString(date);

};

comment:11 Changed 7 years ago by Kenneth G. Franqueiro

Hm, I'm wondering if this is actually by design - see 3 paragraphs into the Data Mapping section in the reference guide page: http://dojotoolkit.org/reference-guide/1.8/dojox/calendar.html#data-mapping

That said, I woulda never guessed that's what was tripping this up to begin with. Yeesh.

comment:12 Changed 7 years ago by ben hockey

i'm going to close this ticket and if any changes are need to dojox/calendar https://github.com/damiengarbarino/dojo-calendar/issues/15 can track those.

comment:13 Changed 7 years ago by ben hockey

Resolution: invalid
Status: newclosed

for the sake of avoiding some confusion... https://github.com/damiengarbarino/dojo-calendar/issues/39 was the actual issue i commented on regarding what i was able to find.

also, i'm closing with a status of invalid since it isn't a valid issue with respect to the store.

comment:14 Changed 7 years ago by cjolif

thanks all. As kgf said this is "by design" and documented. If you don't provide Date objects you have to plug a converter. That was working by "chance" (as the conversion was automatically done) on other browsers. That said we will look into possibilities of improving this in future versions to avoid the confusion that took place here.

Note: See TracTickets for help on using tickets.