Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#14811 closed defect (fixed)

Uploader plugins do not work with AMD

Reported by: Adam Peller Owned by: Mike Wilcox
Priority: high Milestone: 1.9
Component: DojoX Uploader Version: 1.7.1
Keywords: Cc: ben hockey
Blocked By: Blocking:

Description

The declare call in Uploader.js redefines the global dojox.form.Uploader. If the return value of require() is used to load the Uploader rather than the global dojox.form.Uploader the original Uploader code will be used, and the upload() method will be a no-op without any plugin code.

I don't think it's possible to redefine the module this way. Instead, some sort of factory may need to be used within the Uploader to call out to the plugin.

        dojox.form.addUploaderPlugin = function(plug){
                // summary:                                                                                                                           
                //              Handle Uploader plugins. When the dojox.form.addUploaderPlugin() function is called,                                  
                //              the dojox.form.Uploader is recreated using the new plugin (mixin).                                                    
                //                                                                                                                                    
                extensions.push(plug);
                declare("dojox.form.Uploader", extensions, {});
        }

Attachments (1)

amd.patch (802 bytes) - added by Adam Peller 8 years ago.
don't use dojox.form.Uploader to reference the Uploader

Download all attachments as: .zip

Change History (34)

comment:1 Changed 8 years ago by Adam Peller

Milestone: tbd1.7.3
Owner: changed from dante to Mike Wilcox
Priority: undecidedhigh
Status: newassigned

comment:2 Changed 8 years ago by Adam Peller

Maybe you can do a require() and mixin on the result?

comment:3 Changed 8 years ago by Mike Wilcox

Is this speculation or something you ran into? I'm using the Uploader in an app at work, and it works fine.

I wouldn't mind an excuse to change the plugin system though. It confuses too many people. But not sure how quick I could do that.

comment:4 Changed 8 years ago by Adam Peller

Something I ran into, using the new loader -- if you switch to using require() to load the Uploader instead of dojo.require and use the reference for Uploader passed in through the callback in define, instead of referencing it as dojox.form.Uploader. You could start by converting the tests in dojox/form/tests to use require instead of dojo.require.

Last edited 8 years ago by Adam Peller (previous) (diff)

comment:5 Changed 8 years ago by Mike Wilcox

In [27899]:

Converted Uploader Test to AMD to see if there were any issues. None found. Refs #14811

comment:6 Changed 8 years ago by Mike Wilcox

In [27900]:

Uploader Tests - removed redundant addOnload. Refs #14811

comment:7 Changed 8 years ago by Mike Wilcox

I converted one of the two tests (the other one is pretty convoluted and needs time). It works fine. Are you doing something out of the ordinary? I couldn't really follow your description.

comment:8 Changed 8 years ago by Mike Wilcox

You are requiring Uploader first, then the plugins right? (Although I see the obvious potential for problems there being that the loader is async)

comment:9 Changed 8 years ago by Adam Peller

I am, though it seems to be loading in order, perhaps by chance. If the order is important, perhaps the plugin should reference the base Uploader as a dependency?

So I need to revise my description one more time, sorry. I'm not using markup. I'm instantiating the Uploader programmatically from JS. By using markup, you're still going through the global dojox.form.Uploader reference, which has the mixin.

comment:10 Changed 8 years ago by Mike Wilcox

In [27901]:

Adding programmatic test for uploader. Refs #14811

comment:11 Changed 8 years ago by Mike Wilcox

Working programmatic test added.

I bet you forgot to call startup() on it.

And oh, I can't pass this up... Don't get me "started" on "startup". Heh.

comment:12 Changed 8 years ago by Adam Peller

Huh. I never call startup() on Uploader (perhaps I should?) either way, and it hasn't been a problem. With this particular method, though, I assure you, uploader is an empty function. No startup is going to fix it. See patch, attached, which describes the usage I was going for. No dojox.* reference, but a direct reference to the module from the loader.

Changed 8 years ago by Adam Peller

Attachment: amd.patch added

don't use dojox.form.Uploader to reference the Uploader

comment:13 Changed 8 years ago by Mike Wilcox

In [27902]:

Fixed Uploader test to use arg ref instead of global ref. (still works Adam!). Refs #14811

comment:14 Changed 8 years ago by Mike Wilcox

Fixed, and I conceded that was a mistake. But it still works.

comment:15 Changed 8 years ago by Vicente Jiménez Aguilar

I'm having some issues too with dojox.form.Uploader. The "upload" method seems to do nothing. I'm going to do some more checks later. But there a list of things that I'm doing differently:

  • Used Google CDN (maybe I need a local blank.html)
  • HTML5 plugin instead of Flash.
  • Not used for multiple files but trying to upload on select: multiple: false, uploadOnSelect: true

comment:16 Changed 8 years ago by Mike Wilcox

vice, this is not a support forum. Please be sure you're carefully following the docs. If this is in fact a bug, open a new ticket and attach a test.

comment:17 Changed 8 years ago by Adam Peller

It sounds like vice may be experiencing the same problem I am. Sorry I didn't create and attach a test in the first place, but when I try the patched programmatic test you made, select a file, and click on the upload button, nothing happens.

comment:18 Changed 8 years ago by Mike Wilcox

Ok. Sorry vice, I didn't realize you had the same problem. FYI, the uploader with CDN has historically been dicey due to the external resources like the SWF and the iFrame.

Adam, I see what you were talking about now (finally!). I'll add comments in the prog test to emphasize that you have to use the full path and not the require reference.

comment:19 Changed 8 years ago by Mike Wilcox

In [27920]:

Added workaround for prog uploader in test. Refs #14811

comment:20 Changed 8 years ago by Vicente Jiménez Aguilar

Apologies accepted.

My intention was to bring some light detailing my particular use case because I have the same issue.

Thanks for the clarification in the example comments! This must resolve my problems. I'll try it later. It would be nice if this information is also included in the documentation.

By the way, the use of 'dojox.form.Uploader' instead of the reference is needed always or only when using CDN?

Last edited 8 years ago by Vicente Jiménez Aguilar (previous) (diff)

comment:21 Changed 8 years ago by Mike Wilcox

Resolution: wontfix
Status: assignedclosed

The Docs have been updated with the AMD issue. I've also set this to wontfix, but it will be fixed before 2.0.

Vice, the need for the global reference is an AMD issue, not a CDN issue. The CDN issue is pretty much based on loading the iframe and SWF resources, which, at least in the case of the SWF (and probably for the iframe), need to be hosted on the same domain as your application. You should find how to reference those resources in the dojoConfig in the code files.

The good news is if you can enforce HTML5 (ergo, Firefox and Safari) you don't have to worry about that. I know it's a tall order, but it's a step forward.

comment:22 Changed 8 years ago by Adam Peller

Milestone: 1.7.32.0
Resolution: wontfix
Status: closedreopened

comment:23 Changed 8 years ago by Adam Peller

I'd argue this should be fixed SRTL

comment:24 Changed 8 years ago by Adam Peller

Cc: ben hockey added

comment:25 Changed 8 years ago by Mike Wilcox

I don't know how to fix it and be backcompat though.

comment:26 Changed 8 years ago by Adam Peller

@neonstalwart suggests it might be possible to use extend, or just mixin right on the object returned by require()?

comment:27 Changed 8 years ago by ben hockey

i looked into it and it isn't going to work the way i thought it would. using extend overrides the current methods rather than "extending" them but there's got to be a better way to do these plugins. redeclaring a class every time doesn't seem like a good idea.

btw, mike there's a typo in your test page now - it doesn't load.

you have a ; after innerHTML when it should be a :

comment:28 Changed 8 years ago by Mike Wilcox

In [27940]:

Fixed code typo. Refs #14811

comment:29 Changed 8 years ago by Mike Wilcox

Thanks, bug fixed.

So, short story long:

The reason it was built the way it is was an attempt to make it easy for the dev to just declare his plugins and use the Uploader otherwise the same way. Well, good intentions gone astray, this didn't really work. I know a lot of devs use this, so it should be considered an overall success, but I get a lot of people inquiring because they get confused by the concept (which I confess follows no Dojo standards).

My new way of thinking I guess isn't that new at all, because it's much like what Dijit does. Provide a: Flash, HTML5, iFrame, and Form uploader, and also an Uploader, which is the master that mixes them all in. That way, the uninitiated can use Uploader with a bit of code bloat, and those who are more advanced can load the plugins they want and or mix them. FWIW, this is what I have planned for Video, which will have several plugin options.

As you might imagine, this is not an easy change. And it also breaks backcompat.

comment:30 Changed 8 years ago by ben hockey

a short term fix might be:

  • Uploader.js

    
            
            
     
    388388               declare("dojox.form.Uploader", extensions, {});
    389389       }
    390390
    391        return dojox.form.Uploader;
     391       return function () {
     392               return dojox.form.Uploader.apply(this, arguments);
     393       };
    392394});

it's a hack but at least it works today rather than needing to wait until 2.0

comment:31 Changed 7 years ago by bill

Component: DojoX FormDojoX Uploader

comment:32 Changed 7 years ago by Mike Wilcox

Milestone: 2.01.9
Resolution: fixed
Status: reopenedclosed

This is fixed. I converted Uploader to use AMD properly which required discarding the plugin approach. All functionality is now included in Uploader. plugins remain for backcompat in order to throw warnings.

comment:33 Changed 7 years ago by bill

For the record: (In [30328]) Fixes: #14768, #15151, #16293, #14914, #14811, #11885, #14075 converted Uploader to use AMD properly which required discarding the plugin approach. All functionality is now included in Uploader. plugins remain for backcompat in order to throw warnings.

Note: See TracTickets for help on using tickets.