denoland / deno_emit

Transpile and bundle JavaScript and TypeScript under Deno and Deno Deploy
https://jsr.io/@deno/emit
MIT License
225 stars 23 forks source link

Inconsistent behavior regarding sourcemaps with tsc and deno bundle #105

Closed yacinehmito closed 1 year ago

yacinehmito commented 1 year ago

In TypeScript, there are three compiler options that relate to source maps:

There are interplay and incompatibilities between these options. Those are not properly taken into account in deno_emit.

I first encountered this when working on #98, which is meant to solve a few source map issues. I found that deno_emit sets inlineSourceMap to true by default and pretty much ignores everything else until it is manually set to false.

I suggested that deno_emit stop emitting inline source maps by default; @dsherret told me that, if it changes, then it is preferable for the default behavior to be consistent with deno bundle, until it is properly sunset.

I ran a program that automatically runs deno bundle, tsc and deno_emit on the same module for all combinations of the aforementioned options and looked at the output. The results are reported in the table below.

My proposal: I think deno_emit should follow the behavior of tsc. By default this will also match deno bundle as deno bundle never emits source maps and tsc doesn't when no options are provided.


Legend:

sourceMap inlineSourceMap inlineSources deno bundle tsc deno_emit¹
not set not set not set ⬜️ ⬜️ 🔳
true not set not set ⬜️ 🔲 🔳
false not set not set ⬜️ ⬜️ 🔳
not set true not set ⬜️ 🔳 🔳
not set false not set ⬜️ ⬜️ ⬜️
true true not set ⬜️ ❌² 🔳
true false not set ⬜️ 🔲 🔲
false true not set ⬜️ 🔳 🔳
false false not set ⬜️ ⬜️ ⬜️
not set not set true ⬜️ ❌³ 🔳
not set not set false ⬜️ ⬜️ 🔳
true not set true ⬜️ 🔲 🔳
true not set false ⬜️ 🔲 🔳
false not set true ⬜️ ❌³ 🔳
false not set false ⬜️ ⬜️ 🔳
not set true true ⬜️ 🔳 🔳
not set true false ⬜️ 🔳 🔳
not set false true ⬜️ ❌³ ⬜️
not set false false ⬜️ ⬜️ ⬜️
true true true ⬜️ ❌² 🔳
true true false ⬜️ ❌² 🔳
true false true ⬜️ 🔲 🔲
true false false ⬜️ 🔲 🔲
false true true ⬜️ 🔳 🔳
false true false ⬜️ 🔳 🔳
false false true ⬜️ ❌³ ⬜️
false false false ⬜️ ⬜️ ⬜️

¹ The output of bundle() in deno_emit; this is assuming that the compiler options are properly forwarded, per #98, as the current version always emit inline source maps.

² error TS5053: Option 'sourceMap' cannot be specified with option 'inlineSourceMap'.

³ error TS5051: Option 'inlineSources can only be used when either option '--inlineSourceMap' or option '--sourceMap' is provided.

yacinehmito commented 1 year ago

I think deno_emit should follow the behavior of tsc. By default this will also match deno bundle as deno bundle never emits source maps and tsc doesn't when no options are provided.

@dsherret What do you think? I have already written the tests to ensure this. Waiting for a go to implement.

yacinehmito commented 1 year ago

I implemented it, as it was pretty straightforward.

I say that the defaults are actually set in deno_ast, meaning that there were three possible places to perform the required validation:

I did it in the TS wrapper because it was very easy, but let me know if another place is better.

I'll submit a PR once #97 is merged.

dsherret commented 1 year ago

I did it in the TS wrapper because it was very easy, but let me know if another place is better.

I just looked at the code and that seems good to do. The emit defaults in deno_ast are set for transpiling, so we should probably leave those as-is.