Add support for compiler option :inline-source-maps

Question

1. refactor emit-source-map and break it into multiple functions
2. fix logic for relative path computation (see strip-prefix-path)
3. add support for :inline-source-maps option
4. add tests

Related: CLJS-1402, CLJS-1901

Answer 1

Comment made by: darwin

Patches:
https://github.com/darwin/clojurescript/commit/de1da63072c8049b9812d41cce750e5a972c0b0b.patch
https://github.com/darwin/clojurescript/commit/2389fb7839a18299d842d381b135a669a7091869.patch
https://github.com/darwin/clojurescript/commit/3ba594d8c7215ef8c8276737b7e5d8008c6b3a98.patch

Answer 2

Comment made by: darwin

Full review: https://github.com/clojure/clojurescript/compare/darwin:inline-source-maps~3...darwin:inline-source-maps

Also please note that the first patch testing original functionality fails in one test because there was a bug in timestamp formatting in :source-map-url case:
https://github.com/clojure/clojurescript/compare/master...darwin:inline-source-maps#diff-55b85385d2d0bfb6dc20d59ed982d5c8L1239

Answer 3

Comment made by: darwin

Today when testing Dirac I realised we need to embed sources contents as well.

Additional patch:
https://github.com/darwin/clojurescript/commit/c1df38f14a33d02fe2d421f80db0b421b17286bb.patch

New review URL: https://github.com/darwin/clojurescript/compare/inline-source-maps~4...darwin:inline-source-maps

Tested in DevTools and works like a charm.

Answer 4

Comment made by: skrat

This would be helpful for us as well.

Answer 5

_Comment made by: arichiardi_

I have run across this one as well by following [this tutorial|https://yogthos.net/posts/2016-12-26-MacchiatoDebugging.html].

Without either this patch or [Dirac's complicated setup|https://github.com/binaryage/dirac-sample/blob/06321f53a34db73c1e9165c2b355e6e20b65ed14/project.clj#L86] it is not currently possible to use {{node --inspect}} and debug correctly. The symptom I see on our side is that source maps are detected but for some reason Chrome DevTools does not show them in the Tree View.

The content of one of it is:


  {"version":3,"file":"\/Users\/user\/cqrs-engine-cljs\/out\/cqrs\/event_store.js","sources":["event_store.cljs"], ...

Answer 6

Comment made by: dnolen

Linking to patches outside of JIRA is not proper for tickets. Please add a single squashed patch to this ticket directly.

Answer 7

Comment made by: darwin

Attached it as a patch file.

Took https://github.com/darwin/clojurescript/compare/inline-source-maps~4...darwin:inline-source-maps.diff and applied it to current master. It applied cleanly without conflicts. Tests are still passing on my machine.

Answer 8

Comment made by: mfikes

Patch no longer applies; needs re-baseline.

Answer 9

Comment made by: darwin

Newly when developing chrome extensions, source-maps got broken again(link: 1). Probably due to some added security restrictions in what Chrome allows Chrome DevTools to "see".

Some people on the interwebs claim that inlined source-maps are a possible workaround:
https://stackoverflow.com/a/54761431/84283

(link: 1) https://bugs.chromium.org/p/chromium/issues/detail?id=931675

Answer 10

Comment made by: dnolen

Just noting that I'm ok with what's proposed. Please re-baseline the patch and I can review in the near future.

Answer 11

Comment made by: darwin

Thanks. I will try to look into it during this week.

Answer 12

Comment made by: darwin

• rebase
• make sure conflicting commit doesn't get lost: https://github.com/clojure/clojurescript/commit/62926332a78e89935f5aa263195fc29cc297c3b7
• use java.util Base64 encoder instead of javax.xml.bind.DatatypeConverter

Answer 13

Comment made by: mfikes

CLJS-1902-2.patch fails in CI (x)

In particular it fails under Windows.

Here is one instance of failure:

FAIL in (test-external-source-maps) (source_maps_tests.clj:79) 1103source maps with :source-map-asset-path under :none optimizations 1104expected: (check-file (build-result out "main.js") ["sourceMappingURL=http://localhost:1234/some/path/source_maps/main.js.map" (! "rel=")]) 1105 actual: (not (check-file "C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\cljs-tests-source-maps-build\\source-maps-onone-source-map-asset-path\\source_maps\\main.js" ["sourceMappingURL=http://localhost:1234/some/path/source_maps/main.js.map" #object[cljs.source_maps_tests.NegativeCheck 0x38c5d3ae "negative check for 'rel=' (class java.lang.String)"]]))

CI failure log with other instances: https://ci.appveyor.com/project/mfikes/clojurescript/builds/24471651

Answer 14

Comment made by: darwin

I looked into that windows build and sourceMappingURL in failed tests looks like this (note mixed slashes):

//# sourceMappingURL=http://localhost:1234/some/path\source_maps\main.js.map

The problem is in existing code using {{util/path}}, which is OS-dependent and produces back slashes under Windows, my patch didn't modify that code branch:
https://github.com/clojure/clojurescript/blob/47386d7c03e6fc36dc4f0145bd62377802ac1c02/src/main/clojure/cljs/compiler.cljc#L1473

I guess we have two possible resolutions:
1. leave existing behavior as is and make my tests accept mixed slashes
2. newly use forward slashes only, leave my tests intact and announce it as a potentially breaking change in next release

Solution 2 is better way forward IMO. I believe mixed slashes work because browsers are forgiving and treat backslashes as forward slashes in source mapping resolution. This change would have some tiny chance of breaking existing code with would depend on specific way how clojurescript generated sourceMappingURL with backslashes up until now.

I will leave it for your consideration. And then move forward with your decision.

Answer 15

Reference: https://clojure.atlassian.net/browse/CLJS-1902 (reported by darwin)