Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preview should reference Dojo from user's lib/, not app/ #2708

Closed
peller opened this issue Jul 9, 2012 · 32 comments

Comments

Projects
None yet
4 participants
@peller
Copy link
Member

commented Jul 9, 2012

Refs #1586

@JonFerraiolo @rbackhouse

Zazl is unable to find the dojo.js in the preview iframe (preview.html) because it begins with app/ Therefore, the optimizer does not run. Using the Dojo in lib/ is the right thing to do anyway, I think, since it's the version that the user will ultimately see, rather than the library belonging to the workbench.

Problem is, the path to Dojo involves the username, so it must be generated. Is there a way to generate this on the server?

@childsb

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2012

there's no easy way to inject a user/workspace/dojo url from the server. we tried it once, but it was a very messy hack.

so the preview.html's outer dojo call is just for utility, and the users workspace dojo will eventually get loaded. though its awkward, its more inline with workbench utilities (including the singlepreview.js module it loads from the maqetta workbench namespace) than the users files.

can we only zazl the users html file and skip the preview rapper? i'm not sure what the zazl call itself looks like, but we should be building against ./project1/file1.html, and not prevew.html

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2012

zazl is called from the page, not the other way around, so it relies on the page to have the proper path for its script reference

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2012

@childsb points out that the reference is correct. It's for the silhouette. We want zazl to process the document within the iframe which does have lib/* references.

The MaqettaZazl filter is only supposed to operate on urls matching maqetta/user, so it should be filtering the inner document and not the outer document. This line in the filter says so:

httpService.registerFilter("/maqetta/user/*.html", maqettaHTMLFilter, null, null)

Wonder why it's not working?

@ghost ghost assigned peller Aug 25, 2012

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2012

@rbackhouse, can you take a look? To test, simply open the file Sample3-Mobile.html in the samples folder. Hit the preview button on the far right (looks like a triangle/play now) on the app-test server, it will launch a url something like this:

http://maqetta-app-test.ourdomain/maqetta/?preview=1&device=iphone&file=http%3A%2F%2Fmaqetta-app-test.ourdomain%2Fmaqetta%2Fuser%2FC%2Fws%2Fworkspace%2Fproject1%2Fsamples%2FSample3-Mobile.html&zazl=true

which will will not get processed by zazl, as the dojo url is an app/ reference with a mobile svg silhouette. However, it contains a document in an iframe with a url like

http://maqetta-app-test.ourdomain/maqetta/user/C/ws/workspace/project1/samples/Sample3-Mobile.html?theme=iPhone&0.43635040731169283=1

This content should get processed to use zazl.js, but does not. It has a dojo.js reference of ../lib/dojo/dojo/dojo.js

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2012

@rbackhouse what's more interesting is that if that inner url for Sample3-Mobile.html is passed to the browser independently, I see the zazl.js substituted properly... however, the page fails to render:

Uncaught Error: A module with an id of [] has already been provided zazl.js:395
define zazl.js:395
(anonymous function) _javascript:2
Uncaught TypeError: Cannot read property 'dependencies' of undefined zazl.js:292
_loadModuleDependencies zazl.js:292
_load zazl.js:142
(anonymous function) zazl.js:619
script.onload zazl.js:208
@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2012

After staring at this a while, I discovered the obvious: zazl=true is not conveyed to the inner URL. The above zazl exception still needs a look, though. Thanks.

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Aug 27, 2012

The zazl=true is now being carried over as &zazl=true e.g

http://localhost:8081/maqetta/user/maqettaUser/ws/workspace/project1/samples/Sample3-Mobile.html&zazl=true

It needs to be ?zazl=true

I'll take a look at the failure though

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Aug 27, 2012

dojox/mobile/deviceTheme is invalid :

(typeof define === "undefined" ? function(deps, def) { def(); } : define)([
"dojo/_base/config",
"dojo/_base/lang",
"dojo/_base/window",
"require"
], function(config, lang, win, require){

The ')' is in the wrong place, it should be at the end of the file

define)([
...
});

should be

define([
...
}));

With that change Zazl loads the page

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2012

I know it's odd, but I think the syntax of deviceTheme is correct and intentional. I can see how this would make static analysis a little tough.

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2012

if it's not possible to parse this file, could zazl continue, leaving it out of the layer, and load it as a separate entity?

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Aug 27, 2012

The AST parser is not able to locate this define call. One of the tasks of the Zazl optimizer is to assign module ids. As the define is not located no id is assigned. Hardcoding a module id in dojox/mobile/deviceTheme appears to resolve this problem and leave the code structure as is :

(typeof define === "undefined" ? function(deps, def) { def(); } : define)("dojox/mobile/deviceTheme",[
"dojo/_base/config",
"dojo/_base/lang",
"dojo/_base/window",
"require"
], function(config, lang, win, require){

Would this be an acceptable change for this module ?

@jhpedemonte

This comment has been minimized.

Copy link
Member

commented Aug 27, 2012

We're pulling this code straight from the Dojo repo, right? So our ability to change that file is diminished. We'd have to open up a bug report, etc.

Also, Dojo is going away from hard-coded module IDs, since it makes the code not relocatable, using AMD.

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Aug 27, 2012

This not really standard "define" syntax. Any optimizer/code analyzer is going to have issues with this sort thing. Anonymous modules don't work with optimizers, they have to add ids in and to do that they have to identify the point in the code where the id is to be placed.

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2012

I'm ok hacking this into 1.8.0 and getting an approved fix in 1.8.1, if that's our only choice. @rbackhouse, is ignoring the module an option if it doesn't parse?

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Aug 27, 2012

The module does parse. The problem is that the "define" call is not located by the optimizer in the server, the result being that no id is inserted and none of it dependencies are added to the list to be included in the js stream. As it happens all of it dependencies are dependencies that other modules have. The Zazl AMD loader expects that when its "define" function is called that an id is in the signature as it cannot depend on the request URL to resolve the id itself (there is no request that is made to load the module). Without hardcoding the id the call to define will always fail. Ignoring the module doesn't really help here.

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2012

Created a Dojo ticket for @rbackhouse's suggested workaround. I don't remember why the optional define was needed in the first place.

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2012

so patching the Dojo source is problematic, since we're relying on submodules to pull it in. It is possible to patch in a module with paths, but that would mean changing the page structure as well as pulling in dependencies for the user which go beyond the basic library... also problematic. Is there any way to work around this, even if it's a special case in the loader, like looking for a ternary expression with define as a result? icky?

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2012

one other idea... I just realized I could inject paths into the zazl-generated page, but perhaps not inflict that on the user.

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Aug 28, 2012

Looking for the ternary expression with define I'm sure is possible but may be quite a bit of work. The pathing option would probably have to do the whole dojox/mobile prefix. Config Maps is probably the best solution (https://github.com/amdjs/amdjs-api/wiki/Common-Config map) but I haven't got support in yet for it.

None of these are going to be a quick solution. I'm leaning towards going the AST parser change route.

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2012

any more temporary fixes we can apply, like inserting a filter to do search/replace on that ternary string? (for all practical purposes, within our app, we can ditch the ternary and just replace it with "define")

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Aug 28, 2012

Or how about just create a maqetta version and reference that in the generated code for now. I don't think it is referenced anywhere else

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2012

yah, guess I could do that.

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2012

for M8, we might want to try pulling in dojox/mobile/deviceTheme with a SCRIPT tag instead of a require, as recommended by @ykami. This would get us around the zazl issue, since deviceTheme wouldn't go through the loader. However, we may have to provide a special hook for the SCRIPT tag, since we have processing which must occur only after deviceTheme is done loading. We'd also have to worry about maintaining an extra SCRIPT tag in HEAD for this purpose.

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2012

@peller peller closed this Aug 30, 2012

@peller

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2012

creating a branch in the public dojo repository may be a problem. I might have to move to a fork.

peller added a commit that referenced this issue Sep 5, 2012

peller added a commit that referenced this issue Sep 5, 2012

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Oct 6, 2012

I have looked into supporting this in Zazl. One option was that we could use a map configuration to provide an alias to a maqetta version of deviceTheme. Zazl now supports map configurations however to use them the modules involved must be anonymous and as the fix for deviceTheme is to add an id using map configurations won't help us here.

So I have spent a little time looking into making the AST parsing code in Zazl handle this edge case for defining a module. The good news is that the required code to resolve the function name from the conditional expression is not too bad (at least in the java version of the AST parsing code).

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Oct 7, 2012

A new version of Zazl is now available that supports loading deviceTheme.js.

@peller

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2012

I just tried running against the original deviceTheme.js code, and it still fails. @rbackhouse, how do I confirm that I have your zazl changes?

@peller peller reopened this Oct 15, 2012

@rbackhouse

This comment has been minimized.

Copy link
Member

commented Oct 15, 2012

Probably the best way is to look at the Bundle-Version in the MANIFEST.MF for org.dojotoolkit.optimizer.amd.rhinoast.

/maqetta/zazl/optimizer/org.dojotoolkit.optimizer.amd.rhinoast/META-INF/MANIFEST.MF

It should be 0.4.4

@peller

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2012

@JonFerraiolo and I both verified that we have 0.3.6, even though github does point to 0.4.4

https://github.com/zazl/optimizer-release/blob/master/org.dojotoolkit.optimizer.amd.rhinoast/META-INF/MANIFEST.MF

More submodule magic I do not understand?

@jhpedemonte

@peller

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2012

just did a submodule update and some pulls (which seemed to have no impact) and somehow now I am at 0.4.4, and deviceTheme.js works fine. I cloned the tree only 6 days ago, so I don't know why I didn't have the latest to begin with. Wonder if there's something fishy going on with submodules within submodules in git?

@peller

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.