ClojureScript

Send inline source map information for form evaluation

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code

Description

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.

Activity

Hide
David Nolen added a comment - - edited

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.

Show
David Nolen added a comment - - edited 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.
Hide
Nelson Morris added a comment - - edited

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

Show
Nelson Morris added a comment - - edited Second patch attached that uses tools.reader and has some namespace cleanup.
Nelson Morris made changes -
Field Original Value New Value
Attachment inline_source_map2.patch [ 12436 ]
Hide
David Nolen added a comment - - edited

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!

Show
David Nolen added a comment - - edited 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!
Hide
Nelson Morris added a comment -

Attached 3rd patch using everything from tools.reader.

Show
Nelson Morris added a comment - Attached 3rd patch using everything from tools.reader.
Nelson Morris made changes -
Attachment inline_source_map3.patch [ 12443 ]
Hide
Nelson Morris added a comment -

Attached 4th patch rebased against latest master.

Show
Nelson Morris added a comment - Attached 4th patch rebased against latest master.
Nelson Morris made changes -
Attachment inline_source_map4.patch [ 12444 ]
Hide
David Nolen added a comment - - edited

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.

Show
David Nolen added a comment - - edited 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.
David Nolen made changes -
Priority Major [ 3 ] Minor [ 4 ]
Hide
David Nolen added a comment -

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.

Show
David Nolen added a comment - 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.
Hide
Nelson Morris added a comment -

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

Show
Nelson Morris added a comment - No problem. I'll make a new patch once tools.reader has the required functionality.
Hide
Nicola Mometto added a comment -

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.

Show
Nicola Mometto added a comment - 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.
Nelson Morris made changes -
Attachment inline_source_map2.patch [ 12436 ]
Nelson Morris made changes -
Attachment inline_source_map3.patch [ 12443 ]
Nelson Morris made changes -
Attachment inline_source_map4.patch [ 12444 ]
Nelson Morris made changes -
Attachment inline_source_map.patch [ 12434 ]
Hide
Nelson Morris added a comment - - edited

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.

Show
Nelson Morris added a comment - - edited 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.
Nelson Morris made changes -
Attachment inline_source_map.patch [ 12490 ]
David Nolen made changes -
Priority Minor [ 4 ] Major [ 3 ]
Hide
David Nolen added a comment - - edited

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.

Show
David Nolen added a comment - - edited 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.
Hide
Nelson Morris added a comment -

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.

Show
Nelson Morris added a comment - 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.
Nelson Morris made changes -
Attachment inline_source_map.patch [ 12490 ]
Hide
Nelson Morris added a comment -

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.

Show
Nelson Morris added a comment - 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.
Nelson Morris made changes -
Attachment inline_source_map.patch [ 12495 ]
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: