<< Back to previous view

[CLJS-658] Send inline source map information for form evaluation Created: 04/Nov/13  Updated: 29/Nov/13  Resolved: 29/Nov/13

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

Type: Enhancement Priority: Major
Reporter: Nelson Morris Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File inline_source_map.patch    
Patch: Code


Send inline source map information for form evaluation

When evaluating forms from a repl it is nice to see and get errors referencing source information. To achieve that:

1. Send a sourceMappingURL with a data url using a base64 encoded source map
2. Include sourcesContent in the source map based on the form being evaluated
3. Use sourceURL to name the eval block as the generated file referenced in the source map.

The included patch (inline_source_map.patch) provides a working implementation, but some possible downsides:

1. In order to have proper line numbers, handle additional changes based on `wrap`, and present nicely formatted source from just a form, it`pprint`s to a string and reads the input again.
2. It names the generated and source files based on the System/currentTimeMillis.
3. Generating the source maps is not currently optional, and might be useless overhead for non-browser repl-environments.

Comment by David Nolen [ 04/Nov/13 8:07 PM ]

This patch is mostly good however you should not be using the Clojure reader, we now rely on tools.reader as it produces much more accurate line/column information needed by source maps. The patch should be modified to use it.

Comment by Nelson Morris [ 04/Nov/13 8:43 PM ]

Second patch attached that uses tools.reader and has some namespace cleanup.

Comment by David Nolen [ 04/Nov/13 9:04 PM ]

You're still relying on bits of the Clojure reader, I suggest taking a look at cljs/analyer.clj forms-seq to see how it should be done. Thanks much!

Comment by Nelson Morris [ 05/Nov/13 2:24 PM ]

Attached 3rd patch using everything from tools.reader.

Comment by Nelson Morris [ 05/Nov/13 2:51 PM ]

Attached 4th patch rebased against latest master.

Comment by David Nolen [ 05/Nov/13 3:26 PM ]

It appears the patch messes with the Rhino REPL, you should be able to verify this with ./script/repljs. Also can we delete the old patches when we get a new one. Otherwise it gets confusing if we have to revisit this ticket in the future.

Comment by David Nolen [ 06/Nov/13 1:32 PM ]

I reviewed the patch more closely - as cool as this is, it's going to need to wait for tools.reader to preserve original string source information on the forms themselves in order to populate sourcesContent. Also the approach isn't quite right, the source map stuff should only kick in if repl-env has a :source-map flag set to true. This flag can be provided by the browser repl environment.

Comment by Nelson Morris [ 06/Nov/13 7:53 PM ]

No problem. I'll make a new patch once tools.reader has the required functionality.

Comment by Nicola Mometto [ 20/Nov/13 3:19 PM ]

tools.reader now has the required functionality, see https://github.com/clojure/tools.reader/commit/f9e55071d82a443db3fac1c2feda059d0215bb90

A 0.8.0 version will be released soon.

Comment by Nelson Morris [ 26/Nov/13 9:24 PM ]

Previous patches removed and new patch attached that uses tools.reader's source preservation, and checks for a :source-map field in the repl-env.

This passes all tests from `lein test`, runs `./script/repljs` without issue, and will send back sourcemap information from forms evaluated through `.script/browser-repl`.

It did require some changes in how the read-loop occurs. `forms-seq` preserves line numbers between forms and the inline source maps need to start at 0 for each form.

Comment by David Nolen [ 28/Nov/13 8:12 AM ]

Reviewing the patch - I don't understand why read-form works the way it does - given that forms-seq returns a lazy seq this is bound to be a source of error. You need to capture all the stuff at the point at which forms-seq is called, anything else is a bug.

My suggestion is to leave forms-seq alone and supply read-form as a stand alone function.

Also I don't see a comment about the change to compiler.clj why do we need to (dec column) now?

In the future let's avoid whitespace changes in order to simplify reading the patch.

Comment by Nelson Morris [ 28/Nov/13 9:05 AM ]

I expected using `read-form` in `forms-seq` would work the same with regards to bindings as `form-seq` does on master. I'm surprised to hear it might act different. I had hopes `read-form` would also be useful in piggieback/austin, but turns out it wasn't. I'll leave `forms-seq` alone and inline the binding/read into the read loop in repl.cljs. That was the other place it was used.

The `(dec column)` will be needed due to sourcemaps using a 0 index for starting column vs tools.reader using a 1 index. A similar issue to why `(dec line)` is needed a couple lines above. I did not mean for that change to be included as part of this patch, and I'll pull it out into another issue.

I'll attach a new patch with the above changes and ignoring whitespace later today.

Comment by Nelson Morris [ 28/Nov/13 9:04 PM ]

Attached new patch with the minimal changes necessary for browser repl source maps, based on previous comment. It does include one additional change to attempt to get the original input string for forms that are only strings/primitives, and cannot have metadata.

Comment by David Nolen [ 29/Nov/13 3:54 PM ]

fixed, http://github.com/clojure/clojurescript/commit/f02775ebe78ef787f128e37f164763ce492927e8

Generated at Wed Apr 16 08:37:59 CDT 2014 using JIRA 4.4#649-r158309.