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

feat(repl): Type stripping in the REPL #10934

Merged
merged 4 commits into from
Jun 21, 2021

Conversation

dsherret
Copy link
Member

This is part of the implementation for #1158. It allows TypeScript to be inputted in the REPL and it will transpile it to JS for execution.

diagnostic.message,
diagnostic.location.line,
diagnostic.location.col
)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example

image

"'use strict'; void 0;\n{}",
transpiled_src
))
.await
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A side effect of this is when the transformed AST gets printed the line numbers will no longer match.

For example, before (note line number is 2):

image

And after (now it's 5):

image

Is there a way we can fix this easily with the source map since swc can provides that? I'm not super familiar with this, but can look into it if someone believes it's worthwhile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we can fix this easily with the source map since swc can provides that? I'm not super familiar with this, but can look into it if someone believes it's worthwhile.

I don't think it would be that simple - we'd have to store related source map for the last evaluated expression and perform ad-hoc mapping on errors. I think we can skip it for the first pass?

@dsherret dsherret marked this pull request as ready for review June 11, 2021 16:02
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very impressed how little code is required to provide this functionality!

Copy link
Contributor

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class expressions throw parse errors:

class {
  foo() {
  }
  
  bar() {
  }
}

Edit:

Actually, never mind that's consistent with the current behaviour anyway.

@dsherret
Copy link
Member Author

@caspervonb it's the case currently in deno 1.11 as well:

image

@caspervonb
Copy link
Contributor

@caspervonb it's the case currently in deno 1.11 as well:

image

Yeah never mind that note 👍

Copy link
Contributor

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got a panic

> namespace Foo { bar: String }
undefined
> namespace Foo { bar: String }
undefined
> namespace Foo { bar: String = "32" }
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', cli/tools/repl.rs:529:45
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@dsherret
Copy link
Member Author

dsherret commented Jun 11, 2021

@caspervonb interesting! I've opened #10938 to discuss that there (note that bar: is a label in a labeled statement, so what's being done here is re-assigning the String function to "32").

@caspervonb
Copy link
Contributor

caspervonb commented Jun 12, 2021

I'm thinking we should be respecting the media-type as a flag here (e.g, via --ext).
There are some (albeit rare) cases where TypeScript isn't a superset of JavaScript.

None => value,
})
}
Err(err) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store this as last error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's worth the trouble since it's not a thrown error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things that would've been SyntaxError's and stored in _ today are caught at this point so.
If we don't store it's somewhat of a regression.

@dsherret
Copy link
Member Author

I'm thinking we should be respecting the media-type as a flag here (e.g, via --ext).
There are some (albeit rare) cases where TypeScript isn't a superset of JavaScript.

Do you have an example? It might be handled by swc.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@ry ry added this to the 1.12.0 milestone Jun 15, 2021
@dsherret dsherret merged commit 2d2b562 into denoland:main Jun 21, 2021
@dsherret dsherret deleted the repl-type-stripping branch June 21, 2021 19:13
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 this pull request may close these issues.

4 participants