Opened 11 years ago

Closed 8 years ago

#9335 closed defect (wontfix)

[cla] dojo.require with omitModuleCheck breaks debugAtAllCosts

Reported by: Mike Wilson Owned by: Rawld Gill
Priority: high Milestone: future
Component: Loader Version: 1.3.1
Keywords: Cc:
Blocked By: Blocking:

Description

On 1.3.1, trying a little example with omitModuleCheck=true:

  page.html
    dojo.require("dir.mymodule", true);

  dir/mymodule.js
    /* no dojo.provide */
    mystuff = ...;

works fine without debug settings, and with isDebug, but if I configure:

  djConfig = {
    debugAtAllCosts: true,
    isDebug: true
  };

then mymodule is (silently) not loaded.
Seems like a bug?

Best regards
Mike Wilson

Attachments (1)

debugatallcosts.patch (6.2 KB) - added by Mike Wilson 10 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 11 years ago by James Burke

Milestone: tbdfuture
Owner: changed from alex to James Burke

Calling require with omitModuleCheck as true probably will not work in xdomain loading either. The debugAtAllCosts functionality uses the xdomain loader.

I'm a bit torn on this. At the one hand I don't like the omitModuleCheck on dojo.require, and suggest that at least starting in Dojo 1.4 dojo.io.script be used instead (trunk now allows you to dojo.io.script.get() a script and define a load handler on it). On the other hand, omitModuleCheck is part of the API.

I'll have to think more about it. I am tempted to just doc the issue and suggest the fix is to migrate away from dojo.require to dojo.io.script.get() with a load callback. That is an async loading process, but then so is debugAtAllCosts.

comment:2 in reply to:  1 Changed 11 years ago by Mike Wilson

Replying to jburke:

I'm a bit torn on this. At the one hand I don't like the omitModuleCheck on dojo.require, and suggest that at least starting in Dojo 1.4 dojo.io.script be used instead (trunk now allows you to dojo.io.script.get() a script and define a load handler on it). On the other hand, omitModuleCheck is part of the API.

I'd like to ask you to reconsider ;-). I think omitModuleCheck gives important flexibility that needs to be available in some way, though I guess it doesn't have to look exactly the way it does now.

F ex, script.get() will not examine the loaded script to look for further dependencies, so it will not participate in the package system's onload handling.

Also, omitModuleCheck is currently a way to make it possible to have a more flexible file structure (one scope/namespace sub-divided over several files) without littering the namespace with symbols corresponding to every file. I would encourage that the package system allows files that do not define any symbols on their "filename" to still be first-class citizens.

I'll have to think more about it. I am tempted to just doc the issue and suggest the fix is to migrate away from dojo.require to dojo.io.script.get() with a load callback. That is an async loading process, but then so is debugAtAllCosts.

But there is a big difference in shifting over the responsibility to the developer, right? He has to set up a special callback himself, sync with code in onload, can no longer depend on sync loading for !xDomain, etc...

comment:3 Changed 10 years ago by Mike Wilson

I did some more thinking on this subject, and have some more input, but I realized that I need to know what requirements the xDomain loader causes, to be able to recognize module loading and firing onLoad?

comment:4 Changed 10 years ago by Mike Wilson

Here are my thoughts, boiled down to a couple of use-cases that are hard to solve if omitModuleCheck is removed without replacing it with some other solution. It's long, I'm sorry :-/.

Use-case 1: Using the build system to add non-dojo.provide files to layer files

This is an important feature applicable to f ex the recent cometd discussion where cometd wants to avoid any dojo code inside, and would instead be served by a thin dojo wrapper file just doing dojo.provide("dojox.cometd") and dojo.requiring the actual code.

Pointing to the real file with a script.get() would make no good for building the layer file as the semantics of "include package" is gone. Being able to make layer builds is one of the most important reasons to keep a flexible version of the package system "include" command.

Use-case 2: Use debug settings with non-dojo.provide files

Ok, so from use-case 1 we see that we sometimes need to use the package system to refer to non-dojo.provide files. That means they are "inside" the system and would possibly be affected by debug settings etc. This is the part that isn't working currently and I understand from your reply that it may be difficult to solve?

I'm only talking about debug here (note xDomain is handled in use-case 3) and it is quite important to actually get the code loaded so you can go on with your debugging session. I wouldn't mind a lot if non-dojo.provide files were loaded in the normal sync way in debug mode, if it's not possible to consistently load them with script tags (onLoad tracking becoming a mess?). Getting them loaded as evals is much better than not getting them at all ;-).

My point is that it's just a debugging session and has nothing to do with production scenarios, so shortcuts are allowed.

Use-case 3: Requesting non-dojo.provide files using the xDomain loader

Here though we are talking about production scenarios. A production build can be required (in contrast to debug mode) not to have any "loose ends" with free-standing non-dojo.provide files if those are hard to load xDomain. Either the build config or the build system could be responsible for packaging these files inside "layer" files so they are always wrapped in a file that has dojo.provide. This could really just be about issuing an error if non-dojo.provide files are discovered during an xDomain build, and directing the developer to apply use-case 2 above.

So my solution here is to forbid non-dojo.provide files in xDomain builds. An alternative solution could be to allow them, but exclude them from onLoad handling.

Use-case 4: Avoiding junk identifiers when splitting one namespace over multiple source files

Here's a pattern I am successfully using to divide large code files into multiple, more manageable, parts. Let's say I have a lot of code in the namespace mylib.stuff; this would normally go in the file:

  mylib/stuff.js

but to split this in a nice way I instead do:

  mylib/stuff/_api.js
              _init.js
              ...

where each _file.js adds code to the mylib.stuff namespace, and not to its own "file" namespace.

I've started to use this pattern more and more, but have then started to get problems with the ("junk") identifiers declared by dojo.provide, f ex

  dojo.provide("mylib.stuff._api") -> mylib.stuff._api

as these sometimes collide with "real" identifiers on the actual (mylib.stuff) namespace. Adding a new module part file can interfer with existing properties.

My workaround for this has been to either skip using dojo.provides in these files, or to supply the actual namespace instead:

  mylib/stuff/_api.js
    dojo.provide("mylib.stuff")

but that forces me to require them with omitModuleCheck=true.

So this is another use-case where omitModuleCheck is currently needed. A better way of solving this would be to allow to dojo.provide a module without having the unwanted namespace identifiers created, f ex:

  dojo.provide("mylib.stuff", true /*omitNameSpaceCreate*/)

or

  dojo.provide("mylib.stuff._api" /*modulePath*/, "mylib.stuff" /*actualNameSpace*/)

comment:5 Changed 10 years ago by James Burke

mikewse: I would not (at least at this point cannot) remove omitModuleCheck from the API/code. I was mostly just wanting to push off on having to support omitModuleCheck for xdomain/debugAtAllCosts (This is #3 in your use case).

But the more I think about it, I think the way I would want to fix the issue is to make omitModuleCheck irrelevant. IIRC, the main point of it is to make sure a dojo.provide() was in the file. I think that is less important to know than knowing the module at "foo.bar.whatever" has loaded.

So, it would be a bit of work, but I think I see a path to do it. If I made changes so the code basically just cared if the module's JS got loaded, then we would not need omitModuleCheck at all, since everything basically would be omitting the check, and errors would be thrown just if the module JS did not load correctly.

In other words, it would give you the possibility to accomplish the scenarios you list above, and never have to worry about setting omitModuleCheck to true.

It is a bit of work though. Changes in the loader, xd loader and build scripts for things like the code guards would need to sorted out. So until all that gets done, debugAtAllCosts will not be supported with omitModuleCheck = true. However, I'll keep this bug open as a placeholder for the work.

comment:6 Changed 10 years ago by Mike Wilson

I'm all for simplicity, so solving everything and being able to remove configuration sounds good.

So, what you are saying is that you would be removing the requirement of having a dojo.provide call in any file? That sounds much like my thoughts as well, but I can't overview how hard it will get with xDomain loading and such.

About the current problem with debugAtAllCosts: couldn't you just provide an intermediate solution in the meantime, that does sync loading for any resource dojo.required with omitModuleCheck=true? Then the module at least gets loaded into the debugging session which would make things work much better...

comment:7 Changed 10 years ago by James Burke

Trouble is if that omitModuleChecked file does a dojo.require() for something, we have to time it to eval at the right time, and I think since it does not have a dojo.provide call, we are not even tracking it in that sequence where the files are added as script tags serially. Feel free to poke around dojo/_base/loader_xd.js to see if you can get something to work.

comment:8 Changed 10 years ago by Mike Wilson

I made an attempt to fix this, removing the omitModuleCheck thingies and pushing the moduleName all the way down to the XD logic. Everything was looking good, but then I discovered that the actual emptying (shift) of the _xdDebugQueue seems to depend on that the code in a loaded module script tag calls dojo.provide to shift the queue and load the next script or finish the whole thing :-/.

So, if I have not misunderstood this part it seems that a solution for my dojo.provide-less files would need to replace this logic with something else, and then we are probably outside the territory where you would accept a patch from me ;-).

Btw, a general observation is that the XD loading seems maybe unnecessary complicated. It is quite hard to follow the code paths and there seems to be a lot of dependencies. I also found some unused variables so I guess the current codebase has "some history".

comment:9 Changed 10 years ago by James Burke

Yeah it has a bit of history, and needs another pass. I think i made the i18n loading harder than it had to be. but i'm happy to have some other eyes on it. Feel free to suggest things that are weird or could be done better.

comment:10 Changed 10 years ago by Mike Wilson

I guess a lot of the complexity comes from supporting many different scenarios; different modes (sync/debug/xd), packing several modules in one layer, different kinds of resources (i18n), and maybe more?

Do you keep a list of these different use-cases that the system should support? Having that would make it much easier to think about potential improvements...

comment:11 Changed 10 years ago by James Burke

Your list sounds right. I would also clarify that the sync/debug/xd modes could all be operating at the same time: some local modules are loaded sync style, then transformed to fit the xd module format. Then if debugAtAllCosts is on, that mix of modules have to be evaluated in the correct order, but appended as script tags. Those script tags are added in a serial fashion (wait for one to load before doing the next) since in some browsers (IE and Safari) they can execute dynamically added scripts out of DOM order.

comment:12 Changed 10 years ago by Mike Wilson

Ok, so here's a patch with a suggestion for a solution to this issue. Most of the work involves doing as little modifications as possible to the XD loader while still solving the problem.

I needed information about both a module's name and whether it was expected to have a dojo.provide, deep down in the XD loader to be able to do sync fetch for those. This information wasn't available using the current method signatures, so to avoid changing them I instead always pass along the module name (independent of omitModuleCheck) and then in the dependency checking stage detect if there is no provide statement. This information is then carried all the way to loader_debug.js where I can choose to do getText instead of creating a script element.

The solution processes the non-dojo.provide modules much like all other modules, and let them pass through all the stages of the XD loader (previously they were lost quite early).

I have tested quite a few combinations of module dependencies and debugAtAllCosts on/off setting, but not so much with i18n and "real" XD loading as I am not so acquainted with these.

In the code you may want to look out for:

  • Check if I have used "dojoScopeMap" correctly (ie proxy variables instead of "dojo").
  • I have intentionally duplicated some code instead of restructuring to make it easy for you to see the changes, this should probably be refactored.
  • I have removed a couple of local variables.

Observations along the way:

  • loader.js:_loadUriAndCheck: It seems the module check done here duplicates the one in dojo.require, and the one in dojo.require feels like it is in the right place. The only other reference to _loadUriAndCheck is in loader_xd.js, and I suspect it is never called from there, so probably _loadUriAndCheck can be removed.
  • _loadedModules: As the non-dojo.provide modules don't call dojo.provide they create no entry in _loadedModules. It would be nice to put a dummy object in there just to indicate the module is actually loaded, so something like this could be inserted at the end of dojo.require:
    if(...symbol not created...){
    	//We want to be registered in the _loadedModules collection to avoid loading 
    	//this module multiple times
    	dojo._loadedModules[res["resourceName"]] = {};
    }
    

Changed 10 years ago by Mike Wilson

Attachment: debugatallcosts.patch added

comment:13 Changed 10 years ago by James Burke

mikewse: thank you for taking the time to go through the code and make a patch. It might take me a few days to go through it an apply it. But I am keen to get this working because I think it helps us along the way to support cases like ServerJS that do not have a "provide" concept and prefer some locally scoped objects.

comment:14 Changed 10 years ago by Mike Wilson

Ah yes, I was actually thinking about pointing you to the recent discussion about package systems on the ServerJS list, but I guess you are already following that, then ;-). It would certainly be interesting to be compatible with ServerJS in the future.

I should say that I, inspired of your mentioning of the goal of getting rid of dojo.provide, started out in that direction. Though, it proved too hard to pursue without making big changes, so I went back to the "sync fix" solution for this patch.

Btw, another "aspect" for the package system implementation that I noticed while making the patch, is that of debuggability. Ie, I could see design choices where a module eval was put in a certain place to be able to forward compile errors in the module source to the programmer in a natural way.

comment:15 Changed 10 years ago by Mike Wilson

Have you taken any look at this yet, James?

comment:16 Changed 10 years ago by James Burke

Sorry, no I have not. I need to get a nice spot of time to go through it, since it touches some lower level stuff. I am hoping this weekend. However, I am in the middle of packing up for a move at the end of the month. I am loosing more time than I would like to moving concerns at the moment.

comment:17 Changed 10 years ago by Mike Wilson

No worries, don't break yourself. Though, it's interesting for me to know your general impression even before you get the time to actually do any updates to SVN, as this decision sort of affects the structure of my codebase. (I will have to go back to the "currently supported" structure as I want to be able to run with debugAtAllCosts.) So any hints you can give on whether you see mainly problems or mainly upsides of my changes would be welcome :-)

comment:18 Changed 10 years ago by James Burke

My general impression: I was hoping we could use a dynamically create script tag with an onload handler to track the non-dojo.provide module as it attached in debugAtAllCosts mode. As the patch stands now, eval is used for those modules, which really gets around the usefulness of debugAtAllCosts, at least for those modules. An improvement over the current situation, of the page just not loading, but it would be ideal to have something more robust and matching debugAtAllCosts general operation.

I am also not sure the loader.js change to the _loadPath() call is safe for the normal loader case where debugAtAllCosts is true.

I think the changes are in the right general direction, but I would like to see the issue solved by just removing the need for dojo.provide() in general, at least for loader concerns. I am working on a patch for that -- changing the normal dojo loader is pretty straightforward. The harder part are the build processes that depend on provide (buildUtil.addGuardsAndBaseRequires and the xd wrapping code), and the xd loader.

For the build system, knowing what "resource name" corresponds with the current file path (taking prefix dir, swapping for prefix then convert the rest of the path to a dot name) should fix those things.

For the xd loader (and related, debugAtAllCosts), binding to the script onload event with something that informs of the resource name that script tag was for should fix the xd loader issues.

A bit of change, but something I would like to get in for 1.4, time willing.

comment:19 Changed 10 years ago by Mike Wilson

Ah, I mistakenly had the idea that script.onload wasn't available cross-browser. Should I try this out in another patch or have you already covered this bit in your own work?

comment:20 Changed 10 years ago by James Burke

I plan to use it (dojo.io.script on trunk uses it for an example), and I also plan to not need the "noProvide" sections you have in your patch, so be forewarned that my change may supersede/mangle your patch. However, while I want to get this in for 1.4, there are no guarantees my change will land in time since the code is not done yet.

comment:21 Changed 10 years ago by Mike Wilson

If your code is not done yet, and not sure it will get ready for 1.4, I might give it a shot. The aim of my previous patch was actually to change as little as possible in the overall structure while still getting debugAtAllCosts working. I can see how removing the knowledge about dojo.provide at an early stage will simplify things all through all phases (and then use script.onload triggering for all modules) but it will also do a lot more changes than I have done currently. Though, the .noProvide will not be needed of course.

I'm ok with investing the time in this if you state enough of your requirements to give a fair chance that the work I do will not be thrown away. For example, are you thinking that all awareness of dojo.provide should go away, so f ex no module symbol testing will be done in dojo.require? (I'm fine with that but it is interesting to know what scope you imagine for the changes)

Best regards
Mike

comment:22 Changed 10 years ago by James Burke

I am hoping that awareness of dojo.provide in the loader will go away. It will still be useful as an alias for dojo.getObject(), but the loader significance should go away.

dojo.require will know if it loaded by checking for onload events/the response to dojo._getText(), and the dojo.required name will be used to track loaded resources, vs. the provide name.

In that model, there is no more moduleCheck or omitModuleCheck. If there is a failure in the module's definition, the script error for that failure should be enough to track the problem. At least that is the hope.

So given the extent of the change, I am not sure if you would want to keep working on a patch. The one you have now gives a solid base to use if the larger change does not work out.

comment:23 Changed 10 years ago by Mike Wilson

That sounds exactly like the way I would like to go. Personally, I never liked the module checking errors - to me they caused more confusion than help. I'll be happy to take a look, but with this scope it might be good to actually start with the standard loader, to remove the omitModuleCheck stuff there first, and then continue with the XD loader?

One thought: I like the concept of _loadedModules, to track what modules have been loaded. IIRC today it is seeded by dojo.provide, but what about doing a check after each successful module load to see if the corresponding symbol is created, and then add that value to the _loadedModules collection? If the symbol doesn't exist we can still add an "undefined" value to the collection and let the existence of the collection entry mark the "loadness" of the module even when there is no corresponding namespace object.

comment:24 Changed 9 years ago by bill

Owner: changed from James Burke to Rawld Gill

not sure if these tickets are even valid anymore but assigning to rawld to check

comment:25 Changed 9 years ago by Rawld Gill

Status: newassigned

comment:26 Changed 8 years ago by Rawld Gill

Resolution: wontfix
Status: assignedclosed

debugAtAllCosts has been dropped as of v1.6. See #13102 for details.

Note: See TracTickets for help on using tickets.