<< Back to previous view

[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.






[CLJS-851] simplify :none script inclusion if :main supplied Created: 05/Sep/14  Updated: 19/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.





[CLJS-675] QuickStart example not working properly Created: 10/Nov/13  Updated: 15/Oct/14

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

Type: Defect Priority: Minor
Reporter: Anton Logvinenko Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

To reproduce only have to follow simple steps as described in https://github.com/clojure/clojurescript/wiki/Quick-Start
git clone git://github.com/clojure/clojurescript.git
cd clojurescript
./script/bootstrap


Attachments: Text File CLJS-675.patch    

 Description   

Section "Using ClojureScript on a Web Page"
Executing:

./bin/cljsc hello.cljs '{:optimizations :advanced}' > hello.js

produces error message "java.io.FileNotFoundException: out/constants_table.js (No such file or directory)"

Just creating "out" directory seems to be fixing the problem. The directory is then deleted after the compilation.

mkdir out
./bin/cljsc hello.cljs '{:optimizations :advanced}' > hello.js

Also, "out" directory is mentioned in .gitignore.



 Comments   
Comment by Anton Logvinenko [ 10/Nov/13 5:28 AM ]

UPD: The "out" directory is not deleted after the compilation. It's simply ignored by git.

Comment by Steven Kallstrom [ 15/Oct/14 4:05 PM ]

I encountered the same issue. Patch submitted.





[CLJS-857] change deftype*/defrecord* special forms to include their inline methods decls Created: 15/Sep/14  Updated: 15/Oct/14  Resolved: 15/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-change-deftype-defrecord-special-forms-to-include-th.patch     Text File 0001-CLJS-857-change-deftype-defrecord-special-forms-to-i.patch    
Patch: Code

 Description   

Currently the deftype* and defrecord* special forms don't include their inline method declarations, this means that code like:

(deftype foo [x] Object (toString [_] x))

is macroexpanded into something that looks like:

(deftype* foo [x] pmask) (extend-type foo Object (toString [_] x)) ...

The issue with this approach is that we want to treat "x" in the extend-type expression as if it was inside the lexical scope of the deftype*, to do so the analyzer attaches a metadata flag to the extend-type call with the deftype* local fields.

This is not ideal and complicates needlessly the analyzer, I propose to simply move the extend-type call as a parameter to the deftype* special form, this way no special handling for local fields will be needed:

(deftype foo [x] pmask (extend-type foo Object (toString [_] x)))

since the extend-type code is effectively inside the lexical scope of the deftype.

Everything said above applies to defrecord* aswell, this patch implements what I proposed and refactors a bit the code in the analyzer to remove the code duplication in the defrecord* and deftype* parse implementations.

In addition, the current approach was unfeasable to implement for tools.analyzer.js so taking this patch would mean making cljs.analyzer and tools.analyzer.js special-form compatible.



 Comments   
Comment by Nicola Mometto [ 15/Oct/14 9:50 AM ]

Updated patch no longer causes warnings at compile time

Comment by David Nolen [ 15/Oct/14 11:23 AM ]

fixed https://github.com/clojure/clojurescript/commit/9a7f9e8d28a5c57778c821c3816b8dd668328110





Generated at Tue Oct 21 19:26:07 CDT 2014 using JIRA 4.4#649-r158309.