Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#15182 closed enhancement (fixed)

dojo/request

Reported by: Bryan Forbes Owned by: Bryan Forbes
Priority: undecided Milestone: 1.8
Component: IO Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description

Ticket to track dojo/request addition for 1.8.

Attachments (1)

dojo-request-node.patch (2.8 KB) - added by Mark Wubben 7 years ago.

Download all attachments as: .zip

Change History (48)

comment:1 Changed 8 years ago by Bryan Forbes

In [28345]:

Initial check-in of dojo/request. refs #15182 !strict

comment:2 Changed 8 years ago by Bryan Forbes

Milestone: tbd1.8

comment:3 Changed 8 years ago by Bryan Forbes

In [28358]:

Various fixes and unit test updates. refs #15182 !strict

comment:4 Changed 8 years ago by Mark Wubben

Hey Bryan, I've added some notes regarding promises and the Node handler on the GitHub? commit, see the inline notes at <https://github.com/dojo/dojo/commit/33470783157cbd90b11b82c11ab66c4f5c5ace8d>.

dojo/request looks pretty awesome!

Last edited 8 years ago by Mark Wubben (previous) (diff)

comment:5 Changed 8 years ago by Bryan Forbes

In [28362]:

Fix spelling of cancel*. refs #15182 !strict

comment:6 Changed 8 years ago by Bryan Forbes

In [28363]:

Clean up dojo/request/util. refs #15182 !strict

comment:7 in reply to:  4 ; Changed 8 years ago by Bryan Forbes

Replying to markwubben:

Hey Bryan, I've added some notes regarding promises and the Node handler on the GitHub? commit, see the inline notes at <https://github.com/dojo/dojo/commit/33470783157cbd90b11b82c11ab66c4f5c5ace8d>.

dojo/request looks pretty awesome!

I went ahead and implemented the changes for dojo/request/util and the "cancel*" naming changes. The node provider was sort of thrown together as a proof of concept and may be a little crufty. Could you take a stab at making it more robust and attach a patch here? Thanks!

comment:8 Changed 8 years ago by Mark Wubben

Cool. Will have a look at the Node provider, perhaps on the weekend.

Any thoughts on the cancel error?

comment:9 Changed 8 years ago by bill

Keywords: dohfail added

The request.iframe and request.notify tests are both failing on IE8.

comment:10 Changed 8 years ago by Bryan Forbes

Keywords: dohfail removed

I can't reproduce the tests failing on IE8 (I just tested on BrowserStack?). Can you please open a new ticket to report the failures (since this is tracking landing the new code) and attach the console output? Thanks.

comment:11 Changed 8 years ago by bill

OK, I thought that checking it in and making it work should be part of the same ticket, but anyway I filed #15249.

comment:12 Changed 7 years ago by bill

In [28611]:

Document dojo/_base/xhr as deprecated, refs #15182 !strict

comment:13 Changed 7 years ago by Bryan Forbes

In [28679]:

dojo/request updates. refs #15182 !strict

  • Improved watch mechanism.
  • Moved most provider-specific properties from response object to deferred.
  • Added "returnDeferred" parameter to provider methods for back-compat work.
  • Moved script test files into PHP dummy method file.

comment:14 Changed 7 years ago by Bryan Forbes

In [28680]:

fail -> otherwise. refs #15182 !strict

comment:15 Changed 7 years ago by Bryan Forbes

In [28704]:

Compatibility layer for dojo/_base/xhr. refs #15182 !strict

comment:16 Changed 7 years ago by Bryan Forbes

In [28705]:

Improve logic for isValid check. refs #15182 !strict

comment:17 Changed 7 years ago by Bryan Forbes

In [28706]:

Compatibility layer for dojo/io/iframe. refs #15182 !strict

comment:18 in reply to:  15 ; Changed 7 years ago by cjolif

Replying to BryanForbes:

In [28704]:

Compatibility layer for dojo/_base/xhr. refs #15182 !strict

We are not sure of what the exact reason is yet, but this commit does break dojox/app.

comment:19 in reply to:  18 ; Changed 7 years ago by Ed Chatelain

Replying to cjolif:

Replying to BryanForbes:

In [28704]:

Compatibility layer for dojo/_base/xhr. refs #15182 !strict

We are not sure of what the exact reason is yet, but this commit does break dojox/app.

The problem in dojox/app can be seen by running this test: dojox/app/tests/modelApp/index.html and select any of the 3 items in the list. The selections are not working. What I have found is that for the main page, the render function in dojox/app/View.js calls widgetTemplate.buildRendering(); around line 265. buildRendering in _TemplatedMixin calls _beforeFillContent around line 124. _beforeFillContent in _WidgetsInTemplateMixin calls parser.parse around line 30. In parser.parse there is code that looks like this:

var p =
	this._scanAmd(root, options).then(
	function(){ return self.scan(root, options); }).then(
	function(parsedNodes){ return instances = instances.concat(self._instantiate(parsedNodes, mixin, options));});

In the past both .then functions would be called before returning, but after the update to xhr the second .then function is not being called until much later, in the case of the dojox/app test startup is called on the container widget before the children have been created, so startup is not being called on the widgets in the template, and that is why the ListItem? clicks are not working. But I am not sure what is really causing the problem or how best to fix it.

comment:20 in reply to:  19 Changed 7 years ago by Bryan Forbes

Replying to edchat:

Replying to cjolif:

Replying to BryanForbes:

In [28704]:

Compatibility layer for dojo/_base/xhr. refs #15182 !strict

We are not sure of what the exact reason is yet, but this commit does break dojox/app.

The problem in dojox/app can be seen by running this test: dojox/app/tests/modelApp/index.html and select any of the 3 items in the list. The selections are not working. What I have found is that for the main page, the render function in dojox/app/View.js calls widgetTemplate.buildRendering(); around line 265. buildRendering in _TemplatedMixin calls _beforeFillContent around line 124. _beforeFillContent in _WidgetsInTemplateMixin calls parser.parse around line 30. In parser.parse there is code that looks like this:

var p =
	this._scanAmd(root, options).then(
	function(){ return self.scan(root, options); }).then(
	function(parsedNodes){ return instances = instances.concat(self._instantiate(parsedNodes, mixin, options));});

In the past both .then functions would be called before returning, but after the update to xhr the second .then function is not being called until much later, in the case of the dojox/app test startup is called on the container widget before the children have been created, so startup is not being called on the widgets in the template, and that is why the ListItem? clicks are not working. But I am not sure what is really causing the problem or how best to fix it.

I narrowed it down to something in dojo/Deferred. I'm currently working on it with Mark Wubben.

comment:21 Changed 7 years ago by bill

Likely the same thing that is breaking the ContentPane-auto-require test, see #14685.

comment:22 Changed 7 years ago by bill

Presumably the "dojox/app" problem is fixed via [28719].

comment:23 Changed 7 years ago by Mark Wubben

Do you think dojo/request should have added sugar for posting form or JSON data? E.g. take a form kwarg or a json kwarg, rather than the current data?

comment:24 in reply to:  7 Changed 7 years ago by Mark Wubben

Replying to BryanForbes:

Replying to markwubben:

Hey Bryan, I've added some notes regarding promises and the Node handler on the GitHub? commit, see the inline notes at <https://github.com/dojo/dojo/commit/33470783157cbd90b11b82c11ab66c4f5c5ace8d>.

dojo/request looks pretty awesome!

I went ahead and implemented the changes for dojo/request/util and the "cancel*" naming changes. The node provider was sort of thrown together as a proof of concept and may be a little crufty. Could you take a stab at making it more robust and attach a patch here? Thanks!

See <https://github.com/novemberborn/dojo/compare/request-node> and the attached patch. The GitHub? branch has more detailed commits explaining some of the changes.

Changed 7 years ago by Mark Wubben

Attachment: dojo-request-node.patch added

comment:25 Changed 7 years ago by Bryan Forbes

In [28740]:

Fix for older Opera having "addEventListener" but it not firing events. refs #15182 !strict

comment:26 Changed 7 years ago by Bryan Forbes

In [28741]:

Slight change in dojo/request API. refs #15182 !strict

dojo/request calls now return a promise that resolves to the handled
data (similar to the old IO calls). The promise also has a "response"
property; this new new property is a promise that resolves to the object
representing the response and can be used to get information like status
and headers (where applicable).

comment:27 Changed 7 years ago by Bryan Forbes

In [28742]:

Fix typo. refs #15182 !strict

comment:28 Changed 7 years ago by Bryan Forbes

In [28744]:

Implemented compatibility layer for dojo/io/script. refs #15182 !strict

comment:29 Changed 7 years ago by Bryan Forbes

In [28745]:

Make sure notify handler is chained before anything else. refs #15182 !strict

comment:30 Changed 7 years ago by Bryan Forbes

In [28746]:

Added sync test. refs #15182 !strict

comment:31 Changed 7 years ago by Bryan Forbes

In [28747]:

Hooked up /dojo/io/start and /dojo/io/send to dojo/io/iframe. refs #15182 !strict

comment:32 Changed 7 years ago by Bryan Forbes

In [28795]:

Add errors dojo/request will use. refs #15182 !strict

comment:33 Changed 7 years ago by Bryan Forbes

In [28796]:

Implement new errors for dojo/request. refs #15182 !strict

comment:34 Changed 7 years ago by Bryan Forbes

In [28808]:

Added getHeader() to response object. refs #15182 !strict

comment:35 Changed 7 years ago by Mark Wubben

Bryan, did you see my comments from about 8 days ago? Especially the Node patch?

comment:36 Changed 7 years ago by Bryan Forbes

In [28839]:

Updates to node provider and test suite. refs #15182 !strict

comment:37 Changed 7 years ago by Bryan Forbes

In [28872]:

Fix dojo/request/iframe#setSrc. refs #15182 !strict

comment:38 Changed 7 years ago by Colin Snover

Status: newassigned

comment:39 Changed 7 years ago by Bryan Forbes

Resolution: fixed
Status: assignedclosed

Closing this as feature freeze for 1.8 is upon us. Any bugs should be filed as separate tickets.

comment:40 Changed 7 years ago by Douglas Hays

regression tracked in #15524

comment:41 Changed 7 years ago by Bryan Forbes

In [29185]:

Remove sync require for dojo/request/notify and change it to an Evented interface. !strict refs #15182

comment:42 Changed 7 years ago by bill

In [29189]:

The vote in the meeting leaned in favor of removing the deprecation warning, since it displays just by using dojo/store and we can't change that until 2.0. Refs #15182 !strict.

comment:43 Changed 7 years ago by bill

In [29191]:

Put summary where doc parser can get it. Refs #15182 !strict.

comment:44 Changed 7 years ago by liucougar

In [29200]:

refs #15182: specify method POST in uploader iframe plugin

dojo/request/iframe somehow defaults to method GET

comment:45 Changed 7 years ago by liucougar

In [29520]:

refs #15182: specify a meaningful string when creating RequestTimeoutError?

object passed into Error constructor will be converted to string "[object Object]", which is not very helpful

!strict

comment:46 Changed 7 years ago by Bryan Forbes

In [29521]:

Properly call RequestTimeoutError?. refs #15182 !strict

comment:47 Changed 7 years ago by Bryan Forbes

In [29522]:

Properly call RequestTimeoutError?. refs #15182 !strict

Note: See TracTickets for help on using tickets.