Opened 8 years ago

Closed 7 years ago

#14465 closed defect (wontfix)

[patch] [cla] dojox.storage in async mode

Reported by: Martin Repta Owned by: dylan
Priority: low Milestone: 2.0
Component: Storage/Flash Version: 1.7.1
Keywords: dojox, storage Cc: Jens Arps
Blocked By: Blocking:

Description

dojox.storage system does not work with AMD. It must to be converted into new syntax.

I did this conversion.

Attachments (1)

dojox_storage_patch.txt (15.7 KB) - added by Martin Repta 8 years ago.
patch file for dojox.storage system

Download all attachments as: .zip

Change History (17)

Changed 8 years ago by Martin Repta

Attachment: dojox_storage_patch.txt added

patch file for dojox.storage system

comment:1 Changed 8 years ago by bill

Thanks for the conversion. Re your comment in this ticket description

does not work

is anything actually not working, or that's just a strange wording?

Also, you'll need to file a CLA before we can look at your patch, thanks! Let us know when you send it in.

The other thing is that generally all dependencies should be in the top list, in the define() call, rather than having scattered asynchronous require() calls.

comment:2 Changed 8 years ago by Adam Peller

Component: DojoxStorage/Flash
Owner: changed from Adam Peller to Shane O'Sullivan

comment:3 Changed 8 years ago by Adam Peller

Cc: Jens Arps added

comment:4 Changed 8 years ago by Martin Repta

Hi, my CLA was accepted.

I received email from Aimee Busch <abusch@…> (15.nov 2011) which confirms that my Contributor License Agreement was received and my name was added to the Dojo Foundation's contributor listing.

I have been working with dojox.storage system before dojo 1.7 was released. Everything was working fine. But if you want to use it with AMD, it is not working. The reason is that storage modules have not been rewritten into new syntax and that is what I did.

comment:5 Changed 8 years ago by bill

OK thanks, I just don't see you on http://livedocs.dojotoolkit.org/developer/contributors, guess they forgot to add you.

I'm not sure why dojox.storage wouldn't work with AMD, in theory you can load legacy modules from an AMD modules. But anyway AMD conversion is good.

Like I said above though, those random require() calls look dodgy since they execute asynchronously, after the module has finished loading. Is there a reason you didn't list all the dependencies in the standard way?

comment:6 Changed 8 years ago by Martin Repta

I am there, my name is Martin Repta.

I can try to provide sample for problematic behaviour. As I remember, there was probably problem with globales used in dojox.storage.manager. I will checkout latest dojox and send the sample.

About the doggy requires, it is just an optimization, some modules are required exactly at the moment, when they are really needed (for example, see line 202 in patch file). If you wish, I can rewrite it to another version, where all modules are written in the top list.

comment:7 Changed 8 years ago by bill

The thing is that you've changed code that used to run synchronously to now run asynchronously, so chances are that you broke something. Like maybe an app will reference dojox.storage.manager expecting all the plugins to be registered, but they aren't registered yet because the user's callback code ran before those registerModule() calls.

comment:8 Changed 8 years ago by Martin Repta

Get the latest version of dojo sources from github (do not use link from CDN). Try to use storage provider with AMD in async mode. It does not work.

My patch solved the problem.

comment:9 Changed 8 years ago by bill

Summary: dojox.storage in async mode[patch] [cla] dojox.storage in async mode

comment:10 Changed 8 years ago by ben hockey

Priority: highlow

comment:11 Changed 8 years ago by ben hockey

Status: newopen

comment:12 in reply to:  description Changed 8 years ago by tsemachh

Replying to martinerko:

dojox.storage system does not work with AMD. It must to be converted into new syntax.

I did this conversion.

The built version(Which turns it to define etc.) is not working as expected , not sure how but it's broken , please fix that ASAP this holds us from moving to AMD

comment:13 Changed 8 years ago by dylan

Milestone: tbd1.8
Owner: changed from Shane O'Sullivan to dylan
Status: openassigned

comment:14 Changed 8 years ago by dylan

First look at this patch, and it's far from complete in converting the flash module to use AMD and modules throughout. It also doesn't update the inline docs, etc.

I've started working on the conversion, but it's not a simple fix.

comment:15 Changed 7 years ago by dylan

Milestone: 1.82.0

We'll need to delay this for 2.0 unfortunately.

comment:16 Changed 7 years ago by Adam Peller

Resolution: wontfix
Status: assignedclosed

dojox/storage is obsolete. The current code will not be ported to 2.0

Note: See TracTickets for help on using tickets.