<< Back to previous view

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

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

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

Attachments: Text File 0001-Emit-require-for-main-namespace-do-deps-file.patch     Text File 0002-Emit-goog.base-configuration-constants.patch     Text File 0003-Emit-goog.base-into-deps-file.patch     Text File 0004-Emit-all-deps-into-a-single-file.patch     Text File 0005-Update-samples-to-new-none-opt-usage.patch    
Patch: Code

 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.

Comment by Andrew Rosa [ 08/Nov/14 7:59 PM ]

Here are the patches to fix this issue. I made smaller patches for each "step" to light the burden of code review.

  • 0001: this issue creates a new compiler option :main, which receives a symbol with the "main namespace" similar to Clojure. If supplied, will emit a require at the end of our custom deps file.
  • 0002: here I put the Closure configuration constants to make the loading works. I set the goog/base.js dir as a relative path so the change will be less intrusive to cljs compiler.
  • 0003: Emit whole goog.base
  • 0004: combine all deps into our single file, to avoid async dependency loading between google closure libs and our deps.
  • 0005: Update all sample documentation and remove the old "*-dev.html" files.

Thomas: thank you for the help and the examples. In the end I removed the need to know absolute paths by just considering we can aways reach the goog/base directory. This leads to less code involved to make the feature available.

Please let me know if there is any problem with those patches, so I could fix them. This is my first contribution to Clojure(Script), so I still need to grasp the workflow here.

Comment by Martin Klepsch [ 27/Nov/14 10:55 AM ]

@Andrew, I just applied your patches and got the Twitterbuzz example running.
You've done great work, lot's of people will appreciate this change.

One issue I encountered doing that:

In the Twitterbuzz Readme there's a statement as follows:

(def opts {:main 'twitterbuzz.core :output-to "samples/twitterbuzz/twitterbuzz.js" :output-dir "samples/twitterbuzz/out"})
[...]
The reason we set the `:output-dir` is because the `index.html` script tag is specifically pointing to that directory.

When compiling with these options index.html tries to load deps from "[...]clojurescript/samples/twitterbuzz/samples/twitterbuzz/out/twitterbuzz/dom_helpers.js" which is not correct.

After that I just compile with

(def opts {:main 'twitterbuzz.core :output-to "samples/twitterbuzz/twitterbuzz.js" :output-dir "out"})

This places an "out" dir into the clojurescript project directory. Now, when opening index.html, twitterbuzz.js looks for dependencies like this: "[...]clojurescript/samples/twitterbuzz/out/twitterbuzz/dom_helpers.js". This correctly loads the earlier compiled js and the application works. I'm not sure how this is supposed to work as I've mostly just used lein-cljsbuild to date but you probably have an idea

Some minor things

  • The referenced patch CLJS-874 doesn't apply cleanly anymore. I'm too unfamiliar working with patches to help with this unfortunately.
  • Is there any reason to specify :main in advanced compilation mode? (It has been added to the advanced mode section in the twitterbuzz example.)

Really looking forward to see this merged!

Comment by Andrew Rosa [ 29/Nov/14 9:28 PM ]

Thanks @Martin for testing the patches, I kindly appreciate that.

I need to take some time to investigate the issue with this specific example. I'm not confident with the behavior of some examples, like the hello-js one which looks like broken (even on master). Probably I will provide a patch to review and possibly update the examples in another ticket, so we can have a more understandable environment to test this patches.

About the patch CLJS-874, it was made prior the last rework I've made here and probably will not be needed anymore. I need to confess that, if I knew at the time how to do multiple patches I neither need to created it in the first place.

At the last, the :main option isn't really used in advanced compilation. I just put it there to have some sort of "consistency" across all the examples - the only thing that change between them will be the optimization level. Do you think that this generates more confusion than good?

Comment by Mike Thompson [ 30/Nov/14 6:47 PM ]

I love this idea. Ironing out these sorts of stumbling blocks is so important to achieving wider cljs adoption. When I was starting with CLJS at the beginning of the year, I accumulated a healthy set of paper cuts from this sort of stuff. WAY too much time was wasted.

But ... two things:

1. I'd be concerned that the concatenation of base.js and deps.js might slow things down. The lovely thing about `:none` currently is that it delivers sub-second compile times. That's so important to a good productive workflow. But perhaps I'm being paranoid and adding in a base.js pre-pend won't add much time.

2. You really don't need to know what :main is. You can take a sledgehammer to the process by appending the following to the generated file:

```
for(var namespace in goog.dependencies_.nameToPath)
goog.require(namespace);
```

This requires everything in. Everything. (Which is the same outcome as using :whitespace or :simple) And that turns out to be essential in a unit-testing scenario where there isn't a single root namespace. Further explanation:

https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L110-L111

Aside: I worry that this might not be backwards compatible and that it should be achieved by introducing a new `:optimization` target of `:debug` (and deprecating :none ??). Just a thought.

Comment by Thomas Heller [ 01/Dec/14 7:51 AM ]

@Mike Don't worry, prepending base+deps.js is not noticeable performance wise (less than a millisecond).

I tried defining what a "main" is for quite some time in shadow-build but the best I came up with: It's a namespace that contains functions called from outside your CLJS Code (eg. via HTML, aka API functions). It is a very valueable thing to have since its the only way to tell the CLJS compiler (and runtime) which namespaces are actually required. If you have a "main" namespace its deps (and the deps of the deps, ...) can be identified and namespaces that aren't actually needed can be skipped. The Closure dead-code elimination does this for :advanced as well, we just need a little help for :none.

IMHO every app should have at least one "main" ns, requiring everything is a hack not a solution.

As for your clojurescript.test example, that is a shortcoming of the tools.

I'm happy to provide help with shadow-build if anyone is interested, should be pretty simple to add a :test target. To be honest adding more features/options to cljs.closure/build (which lein-cljsbuild uses) is a recipe for disaster as it is already way too complex.

Comment by Andrew Rosa [ 01/Dec/14 10:01 AM ]

I'm also against introducing new options. I really want is eventually deprecate some of the unneeded ones.

I think that the :main namespace is clean, and could avoid issues like having some code evaluated in a namespace that is removed by Closure compiler in production. Not sure how common that could be in ClojureScript, but a valid buggy scenario in terms of global mutable state.

Anyway, if we follow the alternative of requiring everything, I think that could be done without dabbling with goog.base internals. We already have all app namespaces to be able to emit the deps.js

@Thomas your experience with shadow-build has shown you any need of "multiple entry-points" or only a single :main ns is enough? I have a feeling that the need to include all test namespaces is somewhat necessary because how clojurescript.test works, but I'm not familiar how the Clojure version works.

Comment by Thomas Heller [ 01/Dec/14 12:50 PM ]

@Andrew: See my first comment on this issue, in my experience an app will have more than one entry point. "clojurescript.test" would be one example why.

Comment by Martin Klepsch [ 01/Dec/14 3:14 PM ]

@thomas could you further explain the behaviour you'd expect from passing multiple namespaces to :main?

With my current understanding I don't see a need for multiple arguments to the :main option as Andre proposed it.
If you want a build that has another entry point (which also loads tests or similar) why not just make it a different build?

Comment by Thomas Heller [ 01/Dec/14 4:10 PM ]

It's always rather difficult to explain something without a good real world example, but explaining what I do in my app is too app specific. Anyways, my web app has a couple of different features. Lots of different pages and some use one or more features provided by CLJS code. It is not a single-page Application, rather an "old" web app with just some dynamic components sprinkled in.

Let me try an example:

(ns my-app.feature1)
(defn ^:export fancy-component [])

(ns my-app.feature2)
(defn ^:export fancy-component [])

(ns my-app.feature3)
(defn ^:export fancy-component [])

These would all be "main" namespaces by my definition.

I do not want a (ns my-app (:require [my-app [feature1 feature2 feature3]])) since that would couple all components together. One feature provided by shadow-build is that it is able to "split" the produced javascript into multiple files (modules) so you may load them optionally. So I can even split each feature into a seperate .js file loadable on demand, AFTER advanced compilation that is.

I do this in my web app and instead of feeding everyone a hefty 800kb javascript file, everyone gets 90kb and loads the rest when needed. This greatly improved page load times.

The problem with multiple builds is that EVERY build will contain cljs.core, which is quite heavy. You'd end up with

build1: cljs.core, my-app.feature1
build2: cljs.core, my-app.feature2
build3: cljs.core, my-app.feature3

instead of

module1: cljs.core (+ code shared by feature1,2,3)
module2: my-app.feature1
module3: my-app.feature2
module4: my-app.feature3

If you have experience with jQuery, multiple builds would be the equivalent to including jQuery with every jQuery plugin. But I'm getting off topic, thats about modules not about :main.

Hope I made some sense in regards to multiple mains, its just something I use alot but for me it is coupled with modules so YMMV.

Comment by Mike Thompson [ 01/Dec/14 4:15 PM ]

First, I'm a bit surprised. I thought there would be joy at the thought of NOT having to nominate a :main.

At its core, CLJS-851 appears to be about harmonizing `:none` with other options. The initial comment above explains how the HTML files for :none have to be different from the other options :simple etc. We don't like that. BUT the proposal to add :main into project.clj seems set to reintroduce a difference again between :none and the other options, but just in a different place. And there seems to be no need for that difference. :simple and :whitespace bring in all the code. So does :optimized except it is first tree-shaken down by Google Closure. So these 3 options do not need a main. Why should :none have a :main when the other options don't. I can see no downside to pulling in everything and making :none harmonious with the other.

@Andrew I agree that we don't need to use the internals of goog to require everything in. The necessary list of namespaces to require are present at the time of writing deps, so a list of goog.requires could be added, one for each namespace compiled.

Second, @Thomas could you explain more why you think cemerick's framework is wrong to be rootless? In my experience when unittesting you actively do not want a root. You want a directory full of test clj files, with that set of files growing and shrinking over time. Different members of the team might be adding and removing them at each step. You want your tests to be "discovered" by your "test runner" and then executed. I don't want to have to be manually maintaining a central list of tests. In my experience, tools like "Nose" under Python come with a "runner" which discovers all the tests by walking the directory of your project, but that approach isn't an option in the CLJS world. Instead, the compiler has to scoop up all the tests from the directory, compile them and then they have to be "required in", ONLY at that point can the tests be "discovered" in a CLJS world. At least that's my impression.

Proposal: if `:main` isn't there, then bring it all it as per my suggestion. If :main is there then only it gets required.

Comment by Thomas Heller [ 01/Dec/14 4:49 PM ]

@Mike: Without getting too much into the specifics of shadow-build: The feature of discovering tests and running them for you would be trivial, even without making a single change in shadow-build itself.

"... that approach isn't an option in the CLJS world" is simply not true. It is true for cljs.closure/build since its a function combining multiple very complex tasks into one simple map of options. lein-cljsbuild thereby suffers the same fate.

I haven't done much work documenting or promoting shadow-build and to be honest I'd much rather see tools like lein-cljsbuild built on top of shadow-build rather than trying to turn it into a lein plugin itself. Quite frankly to me it is perfect the way it is. I can step into every stage of the CLJS compilation/optimization and do stuff other tools simply cannot do, but that comes at the price of not being very beginner friendly. Documentation might help but I'm really bad at writing that stuff.

Anyways, I would inclue :main for EVERY build, ESPECIALLY :advanced!

I have a rather large codebase (way over 10k LOC CLJS) with several distinct builds. If I had to compile every CLJS file for every build everything would build forever. Some smaller builds just require a couple files (say 10), why feed 300+ into the Closure Compiler to eliminate what a simple :main could have done (and even skipped the CLJS->JS compilation).

I have this stuff in production for about 2 years now, but with custom tools for about as long. I cannot speak for the lein-cljsbuild world whether :main would be a good idea there or not. I don't even care since shadow-build is not affected by this change at all, but given my experience with my own app/codebase I would recommend making :main accept a collection of namespaces.

Comment by Mike Thompson [ 01/Dec/14 5:29 PM ]

@Thomas I'm looking at this through the cljsbuild point of view. I appreciate that shadow build might be able to do test discovery, but unfortunately I don't have the option of using it. So I'm explaining this from the perspective of a cljsbuild user, who is seeking a better outcome.

Comment by Andrew Rosa [ 01/Dec/14 7:33 PM ]

Nice to see this conversation going forward @Mike and @Thomas.

Anyone here can confirm that the advanced compilation behaves like a concatenation of all user defined namespaces? If so, we could change our strategy to mimic the behavior on :none - consistency is a gold IMO, and I like the idea of transparency in optimization levels. Thanks @Mike to point it out.

@Thomas, I guess that the way you suggest to modularize the builds is not possible even for :advanced compilations. I prefer to have a separate issue to discuss and address that feature (which looks interesting for use cases like you presented). And sorry to make you answer the same question again.

The conditional :main is worth the cost of added complexity? How it will affect :advanced compilation?

Comment by Thomas Heller [ 02/Dec/14 4:39 AM ]

Advanced Optimization does not behave like a concatenation, Closure handles the optimization like this (greatly simplified):

1) You provide a list of inputs (Javascript files) and externs
2) Closure parses these into AST inputs
3) basic analysis to extract goog.provide/require information
4) build dependency graph, topological sort
5) magic (if :advanced)
6) generate output

shadow-build does something similar

1) scan all source paths for "resources" (.js, .cljs)
2) extract goog.provide/require from .js
3) read first form of .cljs files (expects it to be 'ns), get provide/require from that
4) build dependency graph, topological sort
5) compile CLJS->JS (if reachable by any :main)
6) feed into Closure Compiler
7) output

Basically cljs.clojure/build does the same, except for #3 which results in CLJS compiling in a somewhat random order (eg. it compiles one 'ns and all its deps until everything is compiled). Without a :main everything will be compiled even if not used anywhere in your project. Closure Advanced Compilation can still remove dead-code since it will analyze the AST and realize that parts (or everything) of a given namespace are never used, :none can not do that without a :main.

If you have 300 source files (CLJS+JS) the load time of a web page (in dev) will be rather high (we are talking seconds here, remember worrying about a couple ms), especially with open dev tools since that will also load an additional 300 source maps. Loading files you don't need therefore is a huge waste of time especially if you reload (or even phantomjs clojurescript.test) often. I cannot stress enough how important :main is for this stuff, but it largely depends on the size of your codebase and the structure of your app. Small example: the Admin "App" of my application will do 714 requests on page load (JS+source maps) and transfer 3.3mb, page load is 3,54s on my rather beefy MacPro (in production this cuts down 13 requests, 343kb and page load in 700ms). Without the :main every page in my app would do 714 requests (or even more) on page load. Not a great development experience. Again I'm using my own tooling but do things like lein-figwheel not trigger full page reloads? REPL might provide a different experience but I found it impossible to properly build/test complex input forms when using a REPL, much rather edit/reload.

@Andrew: Agreed that the module discussion has no place here, just wanted to point out that the :main option becomes mandatory then. To be honest this whole discussion only stresses the problem of cljs.closure/build: trying to combine everyone's expectation of how a build should look into a single function is tough. So in the long run something that lets you split these parts will be easier (hint: shadow-build) but the good part is that ClojureScript itself does not even need to deal with all this stuff. A "simple" CLJS->JS compiler would do, the Closure bits are optional after all (even more so for non-browser targeted builds). But discussing what should be in CLJS and what shouldn't is another issue as well.

Comment by Martin Klepsch [ 02/Dec/14 1:21 PM ]

Again I'm using my own tooling but do things like lein-figwheel not trigger full page reloads?

No. Figwheel only reloads changed files. So the slowdown should be a lot smaller once the page is loaded.

Besides that: @thomas thanks for all your very thoughtful messages here, just reading them already taught me a ton about Clojurescript compilation!

Comment by Andrew Rosa [ 02/Dec/14 6:22 PM ]

Thank you @Thomas for all this explanation, quite insightful. You convinced me that the way to go is requiring :main anyway. If we fell the necessity, we could add the "require all" behavior later. I also like the idea of having a dependency graph to compile only required stuff, but is something for another discussion

Just a final tweak for us to think about: the :main option could receive a list of namespaces or just a single one? Conceptually, I biased for the later. A single namespace could allow us to eventually add something like a -main function, useful for node apps for example - just an idea, of course.

I just updated the patches to fix a single issue with :main not being mungled before require, which caused problems with namespaces including dashes (like foo-bar)

Comment by Mike Thompson [ 02/Dec/14 6:35 PM ]

Okay, my final observations:

  • naming
    Given :main is to be used to indicate a namespace, should its name be changed to make that clearer?
    After all this is about automating: 'goog.require(something)' but the name :main seems unrelated.
    To me :main carries connotations of main functions, not root namespaces.
    I would suggest that ":auto-require" would be a better name and I believe names (cumulatively) matter.
  • Special Marker
    No matter the name chosen, to handle my unittest use-case, would it be possible to have a
    special marker value for :main like, say, "*" which triggers the require everything approach.
    As I've indicated, that would be a big win from where I'm standing.

Thanks to everyone.

Comment by Andrew Rosa [ 02/Dec/14 6:41 PM ]

Good call @Mike. I guess :main was chosen to provide some "parity" with Clojure - that's why I suggest the auto-calling -main feature.

For the unit test case, do you think that it could be done as one enhancement on clojurescript.test? It seems like the test runner should be capable of finding the right namespaces... not sure if it does not do that for limitations of the platform.

Comment by Thomas Heller [ 02/Dec/14 7:16 PM ]

Closure calls them Entry Points, I got used to :main.

Can't really say much about clojurescript.test, only that testing generally is rather complex hiding this behind a "require all" marker will probably not be enough. I know I don't always want to run every single test, usually only those affected by the file I just changed is enough. But getting off-topic again, proper Tooling is hard.

Again for :main, my advice would be a collection but just offer a fallback and turn a single symbol into a collection of one. That covers all cases 0,1,more ... No :main just does not emit any requires which is what we have now.

:main my-app.feature1
:main [my-app.feature1]
:main [my-app.feature1 my-app.feature2]
Comment by Andrew Rosa [ 02/Dec/14 7:27 PM ]

@Thomas no defined :main will still will include base.js and deps.js, but not requiring anything. Right? At least is that what I would expect.

Yeah, tooling is very hard.

Comment by Thomas Heller [ 03/Dec/14 4:20 AM ]

No :main might as well behave like it does now, this way the feature is completely optional and will not break any existing code. Thats usually worth something, don't know how much because as I said, :main is mandatory in shadow-build.

ClojureScript itself should probably be more conservative than that. I imagine there are alot of projects that still use /out/goog/base.js + other in their HTML, that won't change over night.

Comment by David Nolen [ 24/Jan/15 12:25 PM ]

The approaches taken thus far are unnecessarily complex. All that needs to happen is for the generated :none script to document.write the needed scripts and goog.require the :main namespace. I will take a patch that does this.

Comment by Andrew Rosa [ 24/Jan/15 2:24 PM ]

Thanks @dnolen for the feedback. I will work on this patch.

Comment by David Nolen [ 24/Jan/15 2:43 PM ]

fixed https://github.com/clojure/clojurescript/commit/2b6baa2d1e059839a66e2ef90b84f2d025d992dc

Comment by David Nolen [ 24/Jan/15 2:44 PM ]

Andrew, I went ahead and tackled this one.





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

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

Type: Enhancement Priority: Minor
Reporter: Andrew Rosa Assignee: Andrew Rosa
Resolution: Declined 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.



 Comments   
Comment by David Nolen [ 02/Dec/14 5:10 AM ]

Can we get a rebased patch? Thanks.

Comment by Andrew Rosa [ 02/Dec/14 5:24 AM ]

Of course I can provide it @David. But the current CLJS-851 implementation this behavior is not needed anymore. Do you still want this patch or prefer to wait until CLJS-851 resolution?

Comment by David Nolen [ 24/Jan/15 12:20 PM ]

No longer relevant





[CLJS-983] Make ExceptionInfo printable Created: 20/Jan/15  Updated: 24/Jan/15  Resolved: 24/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-983-printable-ex-info-2.patch    

 Description   

Pretty simple enhancement — so we can see what's inside



 Comments   
Comment by David Nolen [ 23/Jan/15 6:21 PM ]

This patch needs a rebased to master.

Comment by Nikita Prokopov [ 24/Jan/15 2:56 AM ]

Things got a little messier after CLJS-985 and prototype manipulations

Comment by Nikita Prokopov [ 24/Jan/15 2:57 AM ]

Uploaded updated patch, rebased to master

Comment by David Nolen [ 24/Jan/15 10:29 AM ]

fixed https://github.com/clojure/clojurescript/commit/7f6f8fd2745a49ac87d3307bdb6e15e91abc26e2





[CLJS-968] Metadata on function literal inside of a let produces invalid Javascript Created: 07/Jan/15  Updated: 23/Jan/15

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

Type: Defect Priority: Major
Reporter: Bobby Eickhoff Assignee: Nicola Mometto
Resolution: Unresolved Votes: 1
Labels: bug
Environment:

Originally found with [org.clojure/clojurescript "0.0-2496"]
Still reproducible with the latest cljsc (b5e9a5116259fc9f201bee4b9c6564f35306f9a5)



 Description   

Here is a minimal test case that produces the invalid Javascript:

(defn f []
  (let [a 0]
    ^{"meta" "data"}
    (fn [] true)))

The compiled Javascript includes the invalid token sequence "return return". (Per Chrome: Uncaught SyntaxError: Unexpected token return)

The problem does not occur if the metadata applies to a map literal instead of a function literal.
The problem only occurs when the function and metadata are inside of a let.



 Comments   
Comment by Bobby Eickhoff [ 07/Jan/15 9:45 PM ]

I forgot to try with-meta. Using with-meta does not produce this syntax error, so it's only a problem with the reader macro for metadata.

Comment by David Nolen [ 08/Jan/15 7:41 AM ]

Any quick thoughts about this one Nicola? Quite possibly a compiler issue on the CLJS side.

Comment by Nicola Mometto [ 08/Jan/15 8:07 AM ]

David, I understand why this happens but I don't know enough about how cljs's js emission to propose a fix.
The issue is that with this commit: https://github.com/clojure/clojurescript/commit/d54defd32d6c5ffcf6b0698072184fe8ccecc93a the following scenario is possible:

{:op :meta
 :env {:context :return}
 :expr {:op :fn
        :env {:context :expr}
        :methods [{:op :fn-method 
                   :env {:context :return} ..}]
        ..}
 ..}

i.e. analyze-wrap-meta changes the context of the :fn node to :expr but keeps the context of the :fn-methods to :return.

This causes both
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L575-L576
and
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L488 (https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L233)

to be true and emit a "return".





[CLJS-966] :foreign-libs should not go through Closure Compiler Created: 07/Jan/15  Updated: 23/Jan/15  Resolved: 23/Jan/15

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Currently the files listed in foreign libs will go through Closure Compiler when using :advanced, they should instead be in the preamble.

https://github.com/thheller/cljs-foreign-bug

lein cljsbuild once release

will create a foreign.min.js [1] which includes the advanced optimized source of js/sample.js (grep for I WONT SURVIVE). In this case the source actually keeps working since the source is quite simple. But we don't even need to include the js/sample.js in the HTML file since the source won't access Foreign.sayHi since it got renamed and uses the optimized code.

[1] https://github.com/thheller/cljs-foreign-bug/blob/master/foreign.min.js



 Comments   
Comment by David Nolen [ 23/Jan/15 6:21 PM ]

fixed in master





[CLJS-985] ex-info loses stack information Created: 20/Jan/15  Updated: 23/Jan/15  Resolved: 23/Jan/15

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File cljs-985-ex-info-stack.patch    

 Description   

Native js Error keeps stacktrace:

(js/console.log (.-stack (js/Error. "message")))

Error: message
    at eval (/Users/prokopov/Dropbox/ws/datascript/test/test/datascript.cljs[eval5]:10:14)
    at eval (native)
    at SocketNamespace.<anonymous> (http://localhost:65000/socket.io/lighttable/ws.js:118:26)
    at SocketNamespace.EventEmitter.emit [as $emit] (http://localhost:65000/socket.io/socket.io.js:633:15)
    at SocketNamespace.onPacket (http://localhost:65000/socket.io/socket.io.js:2248:20)
    at Socket.onPacket (http://localhost:65000/socket.io/socket.io.js:1930:30)
    at Transport.onPacket (http://localhost:65000/socket.io/socket.io.js:1332:17)
    at Transport.onData (http://localhost:65000/socket.io/socket.io.js:1303:16)
    at WebSocket.websocket.onmessage (http://localhost:65000/socket.io/socket.io.js:2378:12)

But ex-info does not:

(js/console.log (.-stack (ex-info "message")))

Error
    at file:///Users/prokopov/Dropbox/ws/datascript/web/target-cljs/cljs/core.js:32066:38

Problem is that ex-info inherits stack property from prototype which is instantiated at script load time here:

(deftype ExceptionInfo [message data cause])

(set! (.-prototype ExceptionInfo) (js/Error.))
(set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)

The possible solution is to create new instance of js/Error at (ex-info) and manually copy stack property to ExceptionInfo object. Related SO: http://stackoverflow.com/questions/783818/how-do-i-create-a-custom-error-in-javascript

Problem is that Chrome has setter on stack property, and it only allows for this property to be set inside a constructor functions.

Proposed fix creates new Error each time ex-info is called and sets ExceptionInfo.prototype to newly created error. This way new ExceptionInfo instance will inherit stack from newly created Error with correct stack.

This patch has been tested in Chrome 39 Mac, Safari 8 Mac, Firefox 35 Mac and IE 10 Win. Here's test code I used:

(defn -ex-info
  ([msg data]
    (set! (.-prototype ExceptionInfo) (js/Error msg))
    (set! (.. ExceptionInfo -prototype -name) "ExceptionInfo")
    (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
    (ExceptionInfo. msg data nil))
  ([msg data cause]
    (set! (.-prototype ExceptionInfo) (js/Error msg))
    (set! (.. ExceptionInfo -prototype -name) "ExceptionInfo")
    (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
    (ExceptionInfo. msg data cause)))

(try
  (throw (ex-info "[ -- Current ex-info message -- ]" 123))
  (catch ExceptionInfo e
    (js/console.log "Current ex-info::" (.-stack e))))

(try
  (throw (js/Error "[ -- Native message -- ]"))
  (catch js/Error e
    (js/console.log "Native error::" (.-stack e))))

(try
  (throw (-ex-info "[ -- Patched ex-info message -- ]" 123))
  (catch ExceptionInfo e
    (js/console.log "Patched ex-info::" (.-stack e))))

Test results:

Chrome, Firefox, IE, Safari

Note that current implementation reports line number and overall stacktrace from cljs.core file where Error prototype is created in current implementation.
Note that patched version reports correct line number (it should be close to native error stack), stack, message and exception name.
Also note that IE is fine even without patch — that's because in IE stack is capturead at throw place, not at new Error() call site.



 Comments   
Comment by Nikita Prokopov [ 20/Jan/15 2:48 PM ]

Ok, this is crazy, but this seems to solve the issue:

(defn ex-info [msg data cause]
  (set! (.-prototype ExceptionInfo) (js/Error msg))
  (set! (.. ExceptionInfo -prototype -name) "cljs.core.ExceptionInfo")
  (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
  (ExceptionInfo. msg data cause))

Basically we change prototype before creating each object.

(taken from http://stackoverflow.com/questions/783818/how-do-i-create-a-custom-error-in-javascript#answer-12030032)

I guess high performance is not needed from ex-info, so this solution is somewhat okay-ish? Should I make a patch from it?

Comment by David Nolen [ 20/Jan/15 2:55 PM ]

It would be nice to get confirmation from others that this works under the major browser - Safari, Firefox, Chrome, and modern IE.

Comment by Nikita Prokopov [ 20/Jan/15 3:12 PM ]

I can confirm Firefox 34, Firefox 35, Safari 8.0.2 and Chrome 39 (all Mac) for now

Comment by Nikita Prokopov [ 22/Jan/15 3:50 AM ]

David, I updated issue, added patch, test code and test results (including IE). There’s no unit test on this because stack traces are very engine-specific. Please take a look

Comment by David Nolen [ 22/Jan/15 2:24 PM ]

Nikita, thanks for the update will check it out.

Comment by David Nolen [ 23/Jan/15 6:13 PM ]

fixed https://github.com/clojure/clojurescript/commit/93dce672e1af8f698cfc2a61e293cb48aeeddc2c





[CLJS-986] Add :target to the list of build options that should trigger recompilation Created: 21/Jan/15  Updated: 21/Jan/15

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

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





[CLJS-27] Conditional compilation (or reading) Created: 22/Jul/11  Updated: 21/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: None

Attachments: File cljs-27.diff     File cljs-27-v2.diff     File cljs-27-v3.diff     File cljs-27-v4.diff     File cljs-27-v5.diff     File cljs-27-v6.diff     File conditional-compilation-clojure.diff     File conditional-compilation-clojurescript.diff    
Patch: Code and Test
Approval: Vetted

 Description   

As people start trying to write libs that are portable between Clojure and ClojureScript they might need to have a bit of branching on target. N.B. supporting this means a change to Clojure, although it has general utility there as well.

Consider CL #+ #- reader macros - http://www.lispworks.com/documentation/lw50/CLHS/Body/02_dhq.htm

Patch: cljs-27-v6.diff

Related: CLJ-1424, TRDR-14



 Comments   
Comment by Roman Scherer [ 19/Jul/12 8:52 AM ]

The following patches include an implementation of Common Lisp's #+
and #- reader macros to allow conditional compilation/reading for
Clojure and ClojureScript.

The patches add a dynamic variable called features to the
clojure.core and cljs.core namespaces, that should contain the
supported features of the platform in question as keywords.

Unlike in Common Lisp, the variable is a Clojure set and not a list.
In Clojure the set contains at the moment the :clojure keyword, and in
ClojureScript the :clojurescript keyword.

I would like to get feedback on the names that are added to this
variable. Are those ok? Is :jvm for Clojure and :js for ClojureScript
better? Should ClojureScript add something like :rhino, :v8 or
:browser as well?

To run the ClojureScript tests, drop a JAR named "clojure.jar" that
has the Clojure patch applied into ClojureScript's lib directory.

Comment by David Nolen [ 19/Jul/12 12:18 PM ]

This is an enhancement so it probably requires a design page and extended discussion before it will go anywhere. Until that happens I'm marking this is as low priority.

Comment by Roman Scherer [ 19/Jul/12 1:45 PM ]

Ok. If someone could give me write access to the Clojure Dev Wiki I would be happy to start such a design page.

Comment by David Nolen [ 19/Jul/12 5:50 PM ]

If you've sent in your CA request permissions on the clojure-dev mailing list.

Comment by Roman Scherer [ 21/Jul/12 5:45 AM ]

I started a design page for this ticket in the Clojure Dev wiki:
http://dev.clojure.org/display/design/Feature+Expressions

Comment by Stuart Halloway [ 27/Jul/12 1:48 PM ]

Posting my comments over on the design page...

Comment by Alex Miller [ 06/Aug/14 7:42 AM ]

Latest patch updates into current ClojureScript and use of tools.reader/tools.analyzer etc. The reader changes are all in the accompanying tools.reader patch in TRDR-14. This patch adds support to allow a new option "features" which is expected to be a set of keywords. build will inject :cljs into this set. The feature set is maintained in clojure.tools.reader/*features*. set! is enhanced to special-case an update to *features* in the code (presumably a rarely-used feature).

Because tools.reader needs the supporting patch, I left in several changes that pull in a new version of tools.reader - I don't actually expect those to be the correct versions but they are there as a reminder to update all of the proper places.

Comment by Alex Miller [ 11/Sep/14 9:04 AM ]

cljs-27-v2.diff adds ability to load .clj files as well as .cljs files when compiling.

Comment by Alex Miller [ 07/Nov/14 10:55 AM ]

Fresh patch, switch to take .cljc files.

Comment by Alex Miller [ 06/Jan/15 3:04 PM ]

Freshened patch for current CLJS.

Comment by David Nolen [ 06/Jan/15 9:55 PM ]

Alex the freshened patch includes modifications to the compiler version dynamic var and compiler version fn. Can we remove these? Thanks!

Comment by Alex Miller [ 07/Jan/15 12:28 PM ]

Yep, updated to -v5 patch.

Comment by Alex Miller [ 21/Jan/15 4:36 PM ]

Updated to use new tools.reader patch and to remove the ability to dynamically set the features set. The active set of features can be set on startup and are held in the features var in the analyzer.





[CLJS-984] Update Node.js REPL support to use public API Created: 20/Jan/15  Updated: 20/Jan/15  Resolved: 20/Jan/15

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

https://github.com/google/closure-library/commit/d909e5aa4b71923d72c15305f70d01a976c9947f



 Comments   
Comment by David Nolen [ 20/Jan/15 2:56 PM ]

fixed https://github.com/clojure/clojurescript/commit/62d898ae30eb58397628b45b3c0c95d3e899a274





[CLJS-890] Incorrect behaviour of (str obj) when obj has valueOf method Created: 24/Nov/14  Updated: 20/Jan/15

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

Type: Defect Priority: Minor
Reporter: Nikita Beloglazov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File cljs-890.patch     File cljs-core-str-perf.diff    

 Description   

Example

(str #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)}) ; => "42"

The problem in the fact that ClojureScript uses concatenation to convert values to strings and that doesn't work well with objects which have valueOf() method overriden.

Example in js:

var obj = {
    toString: function() { return 'hello'; },
    valueOf: function() { return 42; }
};
console.log(String(obj)); => 'hello'
console.log(obj.toString()); => 'hello'
console.log('' + obj); => '42'

Potential solution might be to use String() function. Using toString() won't work as described in this issue: http://dev.clojure.org/jira/browse/CLJS-847



 Comments   
Comment by Kevin Neaton [ 24/Nov/14 10:34 AM ]

Is there a valid use case where toString and valueOf are not in sync? E.g.

(not= (.toString x) (js/String (.valueOf x))

If not, is it "incorrect" for the two methods to be out of sync?

Comment by Nikita Beloglazov [ 24/Nov/14 10:40 AM ]

Here is an example of such use case: https://github.com/processing-js/processing-js/blob/master/src/Objects/Char.js
That's how I found this bug.

Comment by Kevin Neaton [ 24/Nov/14 10:49 AM ]

Thanks for the link. I see what you mean.

Comment by David Nolen [ 02/Dec/14 5:08 AM ]

The problem with going with String appears to be a massive performance hit to printing http://jsperf.com/string-vs-tostring2/6.

Unless a brilliant idea is proposed this seems best solved / worked around in user code.

Comment by Nikita Beloglazov [ 02/Dec/14 6:41 AM ]

Append performs better on strings and numbers, but it performs worse on objects so it is not a clear performance hit. If I heavily work with objects and use (str) to convert them into strings then I actually lose on performance with current implementation.
Anyway current implementation of str is incorrect as it doesn't honor toString method. And this is what str function supposed to do. I believe a compiler should be correct first and then worry about performance.

Comment by David Nolen [ 02/Dec/14 7:38 AM ]

Sorry going back over this I believe the issue is really that we simply need to backout CLJS-801.

Comment by David Nolen [ 02/Dec/14 7:41 AM ]

reverted CLJS-801 in master

Comment by Francis Avila [ 02/Dec/14 10:32 AM ]

CLJS-801 only deals with the str macro. Aren't we still going to have str function problem because of CLJS-847? https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 (also Yet if we use toString there, Safari 6.0.5 blows up. Maybe we need {{[o].join('')}}? Depending on where the bug is this may be wrong in Safari 6.0.5 too.

What we need to do very specifically is somehow get the return value of the (in ECMASCRIPT-ese) ToString abstract operation on the object (or the underlying ToPrimitive abstract operation with the String hint). String concat with the add operator

Options as I see it are:

  • x.toString() : Bad because of CLJS-847
  • {{[x].join('')}} : Should work (and does right thing for null/undefined), but I think we should test in Safari 6.0.5. Also very slow.
  • String
  • String.prototype.concat
  • String.prototype.slice(x,0) String.prototype.substring(x,0) String.prototype.substr(x, 0)
  • x.toString() normally, but String if we detect that we'll trigger CLJS-847. (Can specialize on startup.)
Comment by David Nolen [ 02/Dec/14 10:35 AM ]

Is there any evidence that higher usage of str is actually problematic?

Comment by Francis Avila [ 02/Dec/14 10:44 AM ]

String concat using the addition operator uses an un-hinted ToPrimitive abstract call (which will try x.valueOf() first then x.toString(), usually) and then {{ToString}}s the result of that, so it's not an option unless we are concating primitive values.

Details:

Comment by David Nolen [ 02/Dec/14 10:50 AM ]

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

Comment by Francis Avila [ 02/Dec/14 11:01 AM ]

Is there any evidence that higher usage of str is actually problematic?

Kevin Neaton, who opened CLJS-847, was using a patch in production which only addressed the higher order case and he said the patch fixed the issue for them. He was unaffected by the str macro case because it either used ''+x already (with CLJS-801 applied) or it used {{[x].join('')}} (which hasn't been tested with Safari 6.0.5 yet, but probably works).

So if we had a problem using ''+x with the str macro, we will certainly have a problem with ''+x with a string function as long as CLJS-847 is applied.

I haven't pulled down master yet, but here is a test case which I bet will fail with the CLJS-847 patch:

(def tricky-obj #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)})
(assert (= (apply str tricky-obj) "hello")) ;; will get "42"
Comment by Francis Avila [ 02/Dec/14 11:09 AM ]

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

To be clear, there are two issues here:

CLJS-847: x.toString() fails on Safari 6.0.5. Workaround is ''+x (only done in str macro case).
CLJS-890: ''+x doesn't give expected results for objects which define valueOf. Expectation is that x.toString() is called, instead x.valueOf().toString(). Fix is to use array join instead of string concat in str macro, but it doesn't address the ''+x workaround from CLJS-847.

To make matters worse, it looks like the toString() error on Safari may only be triggered at certain JIT levels!

Comment by Francis Avila [ 02/Dec/14 11:10 AM ]

Workaround is ''+x (only done in str macro case).

I mean "Workaround is ''+x (only done in str function case)."

Comment by Nikita Beloglazov [ 08/Dec/14 6:14 PM ]

Can this bug be reopened meanwhile? If I understand correctly the fix should affect https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 but this code still present in HEAD.

Comment by David Nolen [ 08/Dec/14 6:37 PM ]

We've switched to goog.string.buildString in master https://github.com/clojure/clojurescript/commit/94eb8a960fef6aaca4ba44b251cefbfa04d0f6ac

Comment by Nikita Beloglazov [ 08/Dec/14 8:32 PM ]

Yes, that works. Cool, thanks!

Comment by Thomas Heller [ 01/Jan/15 7:12 AM ]

Sorry for re-opening.

I was doing some profiling of my code and noticed a warning in the profiling output about cljs.core/str.

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

goog.string.buildString = function(var_args) {
  return Array.prototype.join.call(arguments, '');
};

Given that we don't ever call it with more than one argument it is probably not best implementation choice.

Maybe skip the call and inline it ala

(defn str
  "With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args."
  ([] "")
  ([x] (if (nil? x)
         ""
         (.join #js [x] "")))
  ([x & ys]
    (loop [sb (StringBuffer. (str x)) more ys]
      (if more
        (recur (. sb  (append (str (first more)))) (next more))
        (.toString sb)))))

I didn't follow this issue but why are we not using .toString? The buildString/array approach seems kind of hackish?

I'm not too sure about the overall impact but since cljs.core/str showed up pretty high in my profile I think this should be investigated further.

Comment by Thomas Heller [ 01/Jan/15 7:28 AM ]

Before:

;;; str
[], (str "1"), 1000000 runs, 254 msecs
[], (str 1), 1000000 runs, 266 msecs
[], (str nil), 1000000 runs, 80 msecs
[], (str "1" "2" "3"), 1000000 runs, 753 msecs

After:

;;; str
[], (str "1"), 1000000 runs, 82 msecs
[], (str 1), 1000000 runs, 86 msecs
[], (str nil), 1000000 runs, 79 msecs
[], (str "1" "2" "3"), 1000000 runs, 242 msecs

But I only tested V8, probably needs some verification.

Comment by Thomas Heller [ 01/Jan/15 7:39 AM ]
(defn str
  "With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args."
  ([] "")
  ([x1]
     (.join #js [x1] ""))
  ([x1 x2]
     (.join #js [x1 x2] ""))
  ([x1 x2 x3]
     (.join #js [x1 x2 x3] ""))
  ([x1 x2 x3 x4]
     (.join #js [x1 x2 x3 x4] ""))
  ...)

Does perform even better.

;;; str
[], (str "1"), 1000000 runs, 40 msecs
[], (str 1), 1000000 runs, 43 msecs
[], (str nil), 1000000 runs, 96 msecs
[], (str "1" "2" "3"), 1000000 runs, 117 msecs

How many args should it inline?

Comment by David Nolen [ 01/Jan/15 12:43 PM ]

I'd be OK with up to 4 then variadic.

Comment by Thomas Heller [ 01/Jan/15 5:05 PM ]

There is some weird interaction between the code generated by the cljs.core/str macro and function.

The macro generates

(str "hello" 1 "world" :yo nil)

yields

[cljs.core.str("hello"),cljs.core.str((1)),cljs.core.str("world"),cljs.core.str(new cljs.core.Keyword(null,"yo","yo",1207083126)),cljs.core.str(null)].join('');

Given that str with 1 arg will basically unroll to

[["hello"].join(""), ...]

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

Comment by Francis Avila [ 02/Jan/15 11:14 AM ]

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

Chrome complains about the variadic function dispatch code in the same way, see CLJS-916 plus patch.

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

The Closure compiler is not smart enough to remove the intermediate array, which is why I filed CLJS-801 (which this ticket rolled back). I don't think JITs can do it either.

I am beginning to wonder if we should ignore the Safari 6.0.5 problem in CLJS-847 that started all this string mess. To recap:

  1. CLJS-801 is accepted, which removes [123, x].join('') in the str macro case in favor of ''+123+(cljs.core/str$arity$1 x) style code, which the closure compiler can precompute. At this time, the one-arg cljs.core/str function (not macro) calls toString on its argument.
  2. CLJS-847 is filed. On Safari 6.0.5 at higher JIT levels, calling toString on some things (possibly only unboxed numbers? definitely not strings) throws a TypeError. This is unquestionably a bug in Safari. David fixes by making one-arg cljs.core/str function call js-str instead of toString. js-str uses string-concat ''+x.
  3. However, this breaks for objects that define valueOf (issue in current ticket), because in js ''+x is the same as ''+x.valueOf().toString() not ''+x.toString().
  4. David considers using String() and variations but rejects because of performance hit.
  5. David rolls back CLJS-801 from the string-concat back to the array-join style to fix.
  6. Nikita and I point out that rolling back CLJS-801 only fixes the str macro, not the string function, which still uses js-str and hence string-concat.
  7. David fixes the str function to use goog.string.buildString, which has the behavior of array.join. Behavior is now correct even on Safari 6.0.5.
  8. Thomas points out that buildString uses arguments in a way unoptimizable by v8, and now the str function (not macro) has a performance regression. He suggests using [].join() directly.

So, there's a lot of back and forth on this issue, and it's all because of a bug in Safari 6.0.5 which no one has been able to reproduce first-hand because Safari 6.0.5 is old and rare. For some perspective, Safari 6.0.x was only available on Lion and Mountain Lion between July 25,2012 and June 11,2013. Before July 25,2012 Lion used Safari 5.1.x and there was no Mountain Lion. On June 11, 2013, both Lion and Mountain Lion switched to Safari 6.1.x which does not suffer from the toString TypeError bug (I checked--I have an iMac with Lion on it). The only machines on Safari 6.0.5 are (Mountain) Lion machines which used software updates until late 2012-early 2013 and then stopped. I can't imagine this is a large number of people.

It is theoretically possible for me to run Safari 6.0.x on my Lion machine to actually test this, but I can't find a way to downgrade from 6.1.x.

I think the options are:

  1. Use array.join() for all stringification and take the performance hit (which we should quantify). Include a comment that this is only for Safari 6.0.x (only confirmed second-hand on 6.0.4 and 6.0.5) for future generations, who are going to think this is weird.
  2. Use CLJS-801 and toString (status quo before CLJS-847), and ignore this problem for Safari 6.0.x.
  3. Use CLJS-801, but add a number? check (with comment) to cljs.core/str$arity$1 for Safari 6.0.5. The number case should use js-str, and the rest toString. I think this will work, but again we have no way to test--we really need to get our hands on a Safari 6.0.x browser.

Of course we should benchmark these approaches but my hunch is that 2 is faster than 3 is faster than 1.

Comment by David Nolen [ 02/Jan/15 11:16 AM ]

We are not going to ignore Safari 6.0.X. Any decisions made about this ticket will include supporting it.

Comment by Francis Avila [ 10/Jan/15 4:12 AM ]

Update on some research I am doing into this.

I created a jsperf of alternative str implementations that I am trying out. Right now I've only looked at one-arg str. I discovered a few things:

  • {{''+[x]}} is a faster alternative to [x].join('').
  • Advanced compilation can compute {{''+[x]}} at compile time if x is a bool, str, undefined, null, or number, even through function calls! I.e. str_test.str_arr(123) compiles to "123" without macro magic.
  • However, using an intermediate array (even if a preallocated singleton) is still slower than the old (if (nil? x) "" (.toString x))
  • Using a switch statement is as least as fast as the str-tostr baseline, usually faster.
  • I am 99% sure all these implementations (except str-tostr, the baseline, which definitely fails) work on the dreaded Safari 6.0.x. If anyone has this version, point it at the jsperf link above and run the tests. I think Browserstack has this version of Safari.

I'm still investigating the variadic case (str x y z a b c). It might be better to use reduce instead of Stringbuffer+seq. (Stringbuffer just does ''+x now instead of an array-join.)

Comment by Thomas Heller [ 10/Jan/15 6:37 AM ]

Sorry, got side-tracked for a bit.

@Francis: Thanks for the recap.

Don't have Safari 6 available either, but seems wrong that we all have to suffer because is minor percentage still has this (667 users of 190k+ on my site). Don't have a solution cause I can't test whether it works, we might try String.concat.

"".concat(obj); // "42"
"".concat(obj, ""); // "hello"
String.prototype.concat(obj, "") // "hello"
String.prototype.concat("", obj) // "hello"

But no idea if String.concat works, also it behaves odd with regards to valueOf.

http://jsperf.com/js-string-concat-variants

Perf is also inconclusive since Firefox appears to be cheating.

Comment by Francis Avila [ 10/Jan/15 2:04 PM ]

Tested that jsperf with Safari 6.0.5 using Browserstack, results are there.

Note I could not reproduce CLJS-847 because str-tostr does not fail as expected. I will try harder now that I have a browser to test.

Comment by Francis Avila [ 10/Jan/15 6:55 PM ]

Still cannot reproduce CLJS-847.

This script includes my attempt at a minimum reproducible case. My theory was that certain types at higher jit levels would fail. I could not get any case to fail. I also tried flapping back and forth between types and using one type at a time, but still no failures.

In this thread I found this "minimal" script which the OP said he could get to fail reliably. I could not get it to fail. However the original post was from feb 15, 2013, which means the Safari he was using would have to be 6.0.2 or lower.

Hypotheses:

  1. This error does not affect 6.0.5 but maybe 6.0.4 or lower.
  2. BrowserStack's system somehow mitigates the bug, meaning we need a "real" Lion Safari 6.0.x to test.
  3. These tests only fail under the correct phase of the moon.

So I can code up a patch for str using the str-switch implementation (which is at least a bit faster on some browsers), but I have no idea if it may fail on Safari 6.0.5. I only know that it works so far. CLJS-801 should also be safe to reapply because the root cause of all issues is the implementation 1-arity of the cljs.core/str function.

I have also asked for Kevin's help back in CLJS-847. (Kevin was the original reporter of the Safari 6.0.x issue.)

Comment by Francis Avila [ 19/Jan/15 12:51 AM ]

Made a jsperf of variadic cases. Chrome seems to really prefer IReduce to seq+stringbuilder for vectors (other collections not tested), but there is no difference or a small slowdown on other browsers. Not sure if it's worth it.

Also updated arity-one cases with a str using switch and never using toString. Nearly 50% slower than using switch or toString on Chrome, smaller on Safari.

In terms of safety str-switch-notostr does not use toString at all so is probably safer. I think str-switch will likely work too, though, and is significantly faster. However I haven't been able to get any TypeErrors in Safari 6.0.5 so it's anyone's guess.

I suggest something like this as a new str (which doesn't use reduce, but could):

(defn str
 ([x]
  (case (js* "typeof ~{}" x)
   "string" x
   "object" (if (identical? nil x) "" (.toString x))
   ("boolean" "number") (js-str x)
   "undefined" ""
   (js-str #js [x])))                                       ;; insurance against Safari 6.0.x TypeError bug.
 ([a b] (js* "~{}+~{}" (str a) (str b)))
 ([a b c] (js* "~{}+~{}+~{}" (str a) (str b) (str c)))
 ([a b c d] (js* "~{}+~{}+~{}+~{}" (str a) (str b) (str c) (str d)))
 ([a b c d & more]
  (loop [s (str a b c d) [e f g h & r] more]
   (let [s' (js* "~{}+~{}+~{}+~{}+~{}" s e f g h)]
    (if (nil? r)
     s'
     (recur s' r))))))
Comment by Francis Avila [ 19/Jan/15 11:24 PM ]

First cut of a possible patch that resolves this while not breaking CLJS-847. Should wait for confirmation that this does not break Safari 6.0.x.

Comment by Francis Avila [ 19/Jan/15 11:34 PM ]

Oops forgot tests.





[CLJS-847] TypeError in cljs.core/str using Safari 6.0.5 Created: 29/Aug/14  Updated: 19/Jan/15  Resolved: 31/Aug/14

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

Type: Defect Priority: Major
Reporter: Kevin Neaton Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-847.patch    

 Description   

In some versions of Safari 6 (at least 6.0.4 and 6.0.5) calls to cljs.core/str may throw a very cryptic Type Error: type error. This error has occurred repeatedly in our production cljs app over the last ~3 months but I have not been able to reproduce it locally, or boil it down to a simple test case. This appears to be due to the nature of the bug itself. I was however, able to workaround the issue by patching cljs.core/str to use the '' + x form of coercion instead of calling x.toString directly.

Other js projects have encountered this issue and adopted the same solution:

This shouldn't hurt performance and might actually improve it in some browsers:

I'll submit the patch we are using shortly.



 Comments   
Comment by David Nolen [ 31/Aug/14 9:41 AM ]

thanks for the report this is now fixed in master https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642.

Comment by Kevin Neaton [ 31/Aug/14 1:30 PM ]

My pleasure. Thanks for the quick turnaround!

Comment by Nikita Beloglazov [ 23/Nov/14 5:30 PM ]

Seems like using concatenation breaks some cases when valueOf and toString methods are overriden. Example:

var obj = {
    toString: function() { return 'hello'; },
    valueOf: function() { return 42; }
};
console.log(String(obj)); => 'hello'
console.log(obj.toString()); => 'hello'
console.log('' + obj); => '42'

So ClojureScript str function will return '42' for that object, but in fact it should return 'hello'. How about using String() to convert object to strings? I have no idea whether it breaks in Safari or not, may be Safari breaks only on toString() and not String().

Comment by Kevin Neaton [ 23/Nov/14 11:09 PM ]

Since this issue is closed, you might consider creating a new one. That said, I suspect `String` would work as well but I have no way to check, since the original error is so difficult to reproduce.

Comment by David Nolen [ 24/Nov/14 7:27 AM ]

Please open a new issue, thanks.

Comment by Nikita Beloglazov [ 24/Nov/14 9:52 AM ]

Sorry, should have thought about that myself. Done: http://dev.clojure.org/jira/browse/CLJS-890

Comment by Francis Avila [ 10/Jan/15 6:57 PM ]

Kevin, we can't accept your fix because ''+x is not the same as x.toString because the former is like x.valueOf().toString(). We're investigating alternatives but we cannot reproduce your failure. See the end of CLJS-890 for details. If you have a way of reproducing or testing things in Safari 6.0.x your help would be appreciated. A simple thing you could do is open/run the following urls on browsers which gave you trouble:

  1. This file is a minimal reproducible case from this jsbinfrom someone with the same problem outside cljs. It should fail with TypeError: type error on Safari 6.0.x. Can you see if it does? (It doesn't for me on Lion Safari 6.0.5 via BrowserStack.)
  2. This jsperf. The str-tostr should fail on Safari 6.0.5 but I can't get it to fail. Let me know if any of the alternatives work.
  3. Finally, this page is my attempt at a minimal reproducible. I expected at least one of the cases to fail, but again, I can't get any to fail.

Thanks in advance.

Comment by Kevin Neaton [ 11/Jan/15 7:28 PM ]

I have not been able to reproduce this error myself since I do not have access to a computer with Safari 6.0.x. However, this issue has been reproduced (outside of cljs) for other projects experiencing this issue. Based on this and this it seems that the browser console actually needs to be closed for the error to occur. Hopefully, that helps.

Comment by Francis Avila [ 18/Jan/15 9:52 PM ]

We're aware of these other reports of problems (and that the console must be closed because it is JIT-related) but none of them actually provide a test case to reproduce. With some more digging I was able to find this issue which provided this test case (which is the first link in my previous post), but in fact I could not get it to fail on Safari 6.0.5.

How were you able to determine this failed on Safari 6.0.5 and that your fix actually fixed it? Was it all second hand guesswork through these github issues or did you actually see the problem and its resolution? Is there any way for you to confirm that some other patch (other than ''+x) solves this error for you? As it stands we cannot accept your patch but we also cannot reproduce and thus cannot write an acceptable alternative patch.

Comment by Kevin Neaton [ 18/Jan/15 10:31 PM ]

How were you able to determine this failed on Safari 6.0.5 and that your fix actually fixed it?

We noticed the error regularly in production logs for an app running Clojurescript 0.0.2202. The stack trace indicated that the error occurred in native code upon calling toString. The browser version logged along with the errors was always Safari 6.0.5 or 6.0.4. (I can provide you with a sampling of user agent strings that produced the error if that would help). We added the patch above an the error never occurred again.

Is there any way for you to confirm that some other patch (other than ''+x) solves this error for you?

The app that produced the error is now stable and we do not have any updates planned at the moment. I can talk to my boss about deploying an alternate patch to a subset of our users, but it might be a tough sell. Is that what you had in mind?

Comment by Francis Avila [ 19/Jan/15 9:18 AM ]

I can talk to my boss about deploying an alternate patch to a subset of our users, but it might be a tough sell. Is that what you had in mind?

We really need to see this bug in a controlled environment. Deploying a test on the open internet and waiting for stacktraces is not a definitive confirmation of a fix. (How many Safari 6.0.x users do you get anyway? They should be very rare.)

If there is a url in your app that you know raises TypeError, this would be the best way to go (and would not require changing your production code):

  1. With app built with unpatched cljs 0.0.2202: hit page with Safari 6.0.5, confirm TypeError.
  2. With your production code (patched cljs 0.0.2202): hit page with Safari 6.0.5, confirm no TypeError.
  3. With some other patch: confirm no TypeError.

BrowserStack is the only browser testing platform I could find which has Safari 6.0.5, and as I said I could not reproduce the error using it.

One more question: what were your cljs compilation settings?

Comment by Kevin Neaton [ 19/Jan/15 5:05 PM ]

Here are the compiler settings from the build that produced the error:

{:output-to "target/main.js"
 :output-dir "target/main"
 :optimizations :advanced
 :output-wrapper true
 :pretty-print false
 :preamble ["react/react_with_addons.min.js"]
 :externs ["react/react_with_addons.js"]
 :closure-warnings {:externs-validation :off
                    :non-standard-jsdoc :off}}

I had no luck reproducing the issue previously, but I'll take another stab at it later this week and report back here.

Comment by Francis Avila [ 19/Jan/15 11:35 PM ]

There is a new candidate patch in CLJS-890 now.





Generated at Mon Jan 26 08:34:10 CST 2015 using JIRA 4.4#649-r158309.