Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent behavior regarding sourcemaps with tsc and deno bundle #105

Closed
yacinehmito opened this issue Apr 25, 2023 · 3 comments · Fixed by #108
Closed

Inconsistent behavior regarding sourcemaps with tsc and deno bundle #105

yacinehmito opened this issue Apr 25, 2023 · 3 comments · Fixed by #108

Comments

@yacinehmito
Copy link
Contributor

yacinehmito commented Apr 25, 2023

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

  • sourceMap, which controls whether external source maps should be emitted
  • inlineSourceMap, which controls whether inline source maps should be emitted
  • inlineSources, which controls whether the original source code should be embedded in the 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:

  • ⬜️: no source map
  • 🔳: inline source map
  • 🔲: external source map
  • ❌: errors
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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:

  • In deno_ast
  • In the Rust lib of deno_emit
  • In the TS wrapper for deno_emit

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants