<< Back to previous view

[CLJS-876] merged sourcemap doesn't account for output-wrapper Created: 25/Oct/14  Updated: 25/Oct/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: J. David Lowe Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When merging sourcemaps, the cljs.closure/optimize code correctly accounts for "preamble" lines that get prepended to closure compiler output. However, it fails to account for the extra lines added when :output-wrapper is true. As a result, the merged sourcemap is incorrect whenever :output-wrapper is true. (And for extra action-at-a-distance: :output-wrapper is implicitly set to true by lein-cljsbuild whenever :optimizations is :advanced.)

Too tired to make a patch tonight, but I'll go through the CA process and send a patch in a day or two, if nobody gets to it before then.






[CLJS-851] simplify :none script inclusion if :main supplied Created: 05/Sep/14  Updated: 24/Oct/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Andrew Rosa
Resolution: Unresolved Votes: 0
Labels: None


 Description   

If :main namespace supplied - under :none optimization settings :output-to file should goog.require it. This would also allow script inclusion to be unified regardless of the level of optimization supplied, i.e.:

<script type="text/javascript" src="foo.js"></script>

Instead of

<script type="text/javascript" src="out/goog/base.js"></script>
<script type="text/javascript" src="foo.js"></script>
<script type="text/javascript">goog.require("foo.core");</script>


 Comments   
Comment by David Nolen [ 05/Sep/14 5:10 PM ]

This does mean concatenating the contents of goog.base into the deps file.

Comment by Thomas Heller [ 06/Sep/14 8:31 AM ]

Just a quick Note: Not sure what your plan is for :main but it in my experience it is important to make it a collection. A larger app quickly gains multiple entry points.

Comment by David Nolen [ 06/Sep/14 9:38 AM ]

I don't see the point of making it a collection, users can simply load other entry points themselves via the "main" namespace.

Comment by Andrew Rosa [ 14/Oct/14 5:28 PM ]

Is someone already working on this issue? I will familiarize with the compiler code to try tackle this feature.

Comment by Andrew Rosa [ 15/Oct/14 10:17 PM ]

@dnolen I made the changes to compiler for auto require an specified :main namespace (but still optional). After that I started to experiment adding the goog.base code directly into the generated js, just to see what happens.

What I found is that we will need to disable the Closure default deps.js file. So we need to copy the goog.base code and after that include all dependencies, including the Closure ones. This work is need to avoid two issues:

  • We need to explicit set the base path for loading, since the auto-discovery method used by Closure searches for a <script str="base.js">.
  • By default, Closure automatically includes a <script> containing the goog/deps.js file to include itself. This causes race conditions during our require('main-ns'), since only half of our dependencies was added to dependency graph. Explicitly requiring everything will avoid this bug.

I could change the compiler to accommodate all this changes, but first like to communicate you the deep effects on our current behavior. What do you think?

Comment by Thomas Heller [ 16/Oct/14 2:24 AM ]

I have implemented this feature in shadow-build [1] and use the following strategy:

Assuming our output file is named "app.js" for an optimized build, it will look basically like this when compiling with :none.

1) imul.js fix
2) closure global settings

I found that these are required

var CLOSURE_NO_DEPS = true;
var CLOSURE_BASE_PATH = '/assets/js/src/';

The CLOSURE_BASE_PATH is required for the include scripts to find the javascripts for each namespace.

3) After the settings the goog/base.js is emitted into the file.
4) After that the contents of dependency settings are emitted (basically goog/deps.js) plus all of our namespaces. They look something like this

goog.addDependency("clojure/set.js",['clojure.set'],['cljs.core']);

When closure attempts to load a given namespace it will simply emit a <script src="CLOSURE_BASE_PATH + path/to/ns.js">, I found that using an absolute path is the best way to ensure that files are loaded correctly since the "path detection" feature of the goog/base.js file fails since we do not include a goog/base.js

5) Then finally you can just add the

goog.require('main-ns');

Hope that helps.

[1] https://github.com/thheller/shadow-build

Comment by Andrew Rosa [ 16/Oct/14 7:35 AM ]

Thank you Thomas for your very nice writeup.

In my tests I found that we could leave CLOSURE_BASE_PATH alone IF we made everything relative to our output file. Do you think forcing it to some path is more reliable?

Comment by Thomas Heller [ 16/Oct/14 1:08 PM ]

Yes definitly. The location of the javascript files usually is "fixed" once the app is configured. Either you put it locally at something like "/js" or something like a CDN, so you configure it once and forget about it. But thats my own personal opinion, I have no idea how other people are handling it. I had some problems with relative paths but can't remember exactly what they were only that CLOSURE_BASE_PATH fixed it.

Comment by Andrew Rosa [ 24/Oct/14 4:52 PM ]

Just to give an update here: I already have a patch for this issue, but depends on the resolution of CLJS-874.





[CLJS-875] Compiler error on :" Created: 22/Oct/14  Updated: 23/Oct/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Stuart Mitchell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs
Environment:

Ubuntu 14.04
org.clojure/clojurescript "0.0-2277"



 Description   

(str "this will break" :"the parser")
This simple form (note the misplaced ':' causes the compiler to barf with the following error
Caused by: clojure.lang.ExceptionInfo: EOF while reading string

no mention of the ':' and in larger blocks of code the unmatched brackets several lines away is flagged

in clojure the error is
RuntimeException Invalid token: : clojure.lang.Util.runtimeException (Util.java:221)
RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:221)

Note it mentions the errant ':'



 Comments   
Comment by Stuart Mitchell [ 22/Oct/14 11:04 PM ]

I would like the error reporting from the compiler improved in this case, so that it mentions that the problem is caused by the ':'.

Comment by Nicola Mometto [ 23/Oct/14 7:53 AM ]

This appears to be an issue caused by tools.reader, I'm working on a fix for it

Comment by Nicola Mometto [ 23/Oct/14 9:17 AM ]

I just pushed a 0.8.10 release of tools.reader that fixes this issue





[CLJS-874] Add closure/path-relative-to support to relative paths based on directories Created: 19/Oct/14  Updated: 19/Oct/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Andrew Rosa Assignee: Andrew Rosa
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 874.patch    
Patch: Code and Test

 Description   

The referred function could not handle directories as base paths, since it only calculate the relative paths via string manipulation. I propose using some checking on file system (through {File#isDirectory()}) to be able to differentiate directories from files.

The current behavior:

(path-relative-to (io/file ".") {:url (deps/to-url (io/file "cljs/core.js"))})
;; => "core.js"

After the patch we will have:

(path-relative-to (io/file ".") {:url (deps/to-url (io/file "cljs/core.js"))})
;; => "cljs/core.js"

This behavior is needed for my patch for CLJS-851. If there is some better alternative to address that, I'm open to make the appropriate changes.






Generated at Sat Oct 25 10:28:16 CDT 2014 using JIRA 4.4#649-r158309.