-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Javascript es6 migration #2931
Javascript es6 migration #2931
Conversation
@carocad |
hey @ericvergnaud I will take a look later today. I dont have much experience with es6 imports in node.js since they are so new but I can take a look for sure 😉 . |
Cool thanks Setting up nodejs (14.13.0-1nodesource1) ... as if installing node 14 had no effect on which node is used... |
@@ -74,7 +74,7 @@ LANotEquals(i, v) ::= <%this._input.LA(<i>)!=<v>%> | |||
|
|||
TokenStartColumnEquals(i) ::= <%this._tokenStartColumn===<i>%> | |||
|
|||
ImportListener(X) ::= <<var <X>Listener = require('./<X>Listener').<X>Listener;>> | |||
ImportListener(X) ::= "" |
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.
@ericvergnaud if I recall correctly these files are not needed anymore since we dropped the browser test suite in #2755
Agreed
Envoyé de mon iPhone
… Le 5 oct. 2020 à 00:24, Camilo Roca ***@***.***> a écrit :
@carocad commented on this pull request.
In runtime-testsuite/resources/org/antlr/v4/test/runtime/templates/Chrome.test.stg:
> @@ -74,7 +74,7 @@ LANotEquals(i, v) ::= <%this._input.LA(<i>)!=<v>%>
TokenStartColumnEquals(i) ::= <%this._tokenStartColumn===<i>%>
-ImportListener(X) ::= <<var <X>Listener = require('./<X>Listener').<X>Listener;>>
+ImportListener(X) ::= ""
@ericvergnaud if I recall correctly these files are not needed anymore since we dropped the browser test suite in https://github.com/antlr/antlr4/pull/2755/files
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@@ -2,4 +2,8 @@ | |||
|
|||
set -euo pipefail | |||
|
|||
mvn -q -Dparallel=methods -DthreadCount=4 -Dtest=javascript.* test | |||
cd ../runtime/JavaScript | |||
npm install |
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.
npm install |
I think this is not needed since the runtime doesn't have any production
dependencies only for development
.
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.
want to give it a try? you might be right, but if you are the cost is negligible
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.
but if you are the cost is negligible
yeah, fair point. Lets keep it as it is
@parrt blessed |
@ewanmellor |
No description provided.