-
Notifications
You must be signed in to change notification settings - Fork 43
Add parser support for imports #43
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
Conversation
@@ -112,6 +113,11 @@ class TSDefParser extends StdTokenParsers with ImplicitConversions { | |||
lazy val typeAliasDecl: Parser[DeclTree] = | |||
"type" ~> typeName ~ tparams ~ ("=" ~> typeDesc) <~ opt(";") ^^ TypeAliasDecl | |||
|
|||
lazy val importDecl: Parser[DeclTree] = | |||
"import" ~ rep(not(";") ~ anyToken) ~ ";" ^^^ ImportDecl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Er ... no, that does not make much sense. You're trying to lex the import away, à la regex [^;]*;
, but that's a very fragile way to do it. You should instead truly parse the import statement, using its complete grammar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The asterisk used for wildcards import *
is not recognized by the lexer yet so it results in a failure("illegal character")
. It doesn't seem to fit well into any of the existing token types though. What do you think? Should we add a new one or put it into one of the existing nevertheless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be in lexical.delimiters ++=
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to parse imports properly now.
@@ -37,7 +37,7 @@ class TSDefParser extends StdTokenParsers with ImplicitConversions { | |||
|
|||
// Future reserved keywords in Strict mode - some used in TypeScript | |||
"implements", "interface", "let", "package", "private", "protected", | |||
"public", "static", "yield", | |||
"public", "static", "yield", "from", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from
is not a keyword. The import
statements recognize from
as a special identifier, but only in that specific context. In all non-import contexts, from
should stay a regular identifier, otherwise var from: number
will not be valid anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see. Is lexical.Identifier("from")
the right way to parse it in the import context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what is the best way to go about this. accept
+lexical.Identifier("from")
might be a reasonable choice.
samples/import.ts.scala
Outdated
|
||
@js.native | ||
@JSGlobal | ||
object Importedjs extends js.GlobalScope { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis is not happy with that line. Importedjs
should have @JSGlobalScope
+extends js.Any
instead of @JSGlobal
+extends js.GlobalScope
.
(it might not be related to the present PR--the bug might have existed before, in which case you might want to fix it in a separate PR first)
4a973b9
to
120eed5
Compare
samples/import.ts
Outdated
|
||
import * as validator from "./ZipCodeValidator"; | ||
|
||
import "./my-module.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that really valid syntax in TypeScript?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this was directly taken from https://www.typescriptlang.org/docs/handbook/modules.html#import
Although they discourage imports only for side effects it is valid.
"import" ~> ( | ||
"{" ~ identifier ~ opt(lexical.Identifier("as") ~ identifier) ~ "}" ~ lexical.Identifier("from") ~ stringLiteral | ||
| "*" ~ lexical.Identifier("as") ~ identifier ~ lexical.Identifier("from") ~ stringLiteral | ||
| stringLiteral |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing the default import syntax:
import foo from "bar.js"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems I shouldn't have just relied on the TypeScript docs, they missed some obvious examples and so did I. ;)
@@ -112,6 +113,13 @@ class TSDefParser extends StdTokenParsers with ImplicitConversions { | |||
lazy val typeAliasDecl: Parser[DeclTree] = | |||
"type" ~> typeName ~ tparams ~ ("=" ~> typeDesc) <~ opt(";") ^^ TypeAliasDecl | |||
|
|||
lazy val importDecl: Parser[DeclTree] = | |||
"import" ~> ( | |||
"{" ~ identifier ~ opt(lexical.Identifier("as") ~ identifier) ~ "}" ~ lexical.Identifier("from") ~ stringLiteral |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not general enough. It should be able parse multiple identifiers within the braces. For example:
import { foo as bar, foobar, babar as baz } from "module.js"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should work now.
120eed5
to
fd82e9c
Compare
Thanks for guiding me through this :) |
Having to manually remove lots of import statements from with many generated d.ts files can be quite tedious so I've added parser support for imports (just ignoring them though).
I have no experience with parser combinators so far so you might want to have a look if it makes sense at all or if could be done in a simpler way.
I guess this would also fix #12