Clean up of the JS/TS target #4409

mike-lischke · 2023-09-05T12:09:34Z

@ericvergnaud OK, let's start with the non-functional clean up. This patch only contains things that improve formatting + copyrights, as well as a smaller production build. Should be easy to agree on these changes.

Would appreciate a quick review, so I can continue with the following patches.

- Added copyright were missing. - tab -> space conversion. - General formatting. Signed-off-by: Mike Lischke <[email protected]>

Signed-off-by: Mike Lischke <[email protected]>

Everything is good locally. Why's GH actions failing? Signed-off-by: Mike Lischke <[email protected]>

ericvergnaud · 2023-09-05T14:09:25Z

runtime-testsuite/test/org/antlr/v4/test/runtime/typescript/TsNodeRunner.java

@@ -35,7 +35,7 @@ protected void initRuntime(RunOptions runOptions) throws Exception {
 	}

 	private void npmInstallTsNodeAndWebpack() throws Exception {
-		Processor.run(new String[] {NPM_EXEC, "--silent", "install", "-g", "typescript", "ts-node", "webpack", "webpack-cli"}, null);
+		Processor.run(new String[] {NPM_EXEC, "--silent", "--force", "install", "-g", "typescript", "npx", "tsx", "webpack", "webpack-cli"}, null);


We're not switching to tsx. According to their web page, they don't use tsc, and don't perform type checking.
Tsc compatibility is a requirement.

Using tsx is just an attempt to get the build working. As you saw ts-node failed in GH actions and it fails for me locally. I use ts-node for other projects and know it a bit, but it still gives griefs, while tsx works out of the box.

webpack doesn't do any type checking either. During development you would need an extra type checker anyway. VS Code has a project wide TS type checker option, which makes this super easy.

Anyway, I cannot change that back or I will not be able to run the ANTLR4 tests locally.

ts-code runs tsc that does type checking.

But only when you run the tests, not during development where it actually counts!

ericvergnaud · 2023-09-05T14:10:45Z

runtime/JavaScript/src/antlr4/CharStream.d.ts

@@ -1,5 +1,9 @@
-export declare class CharStream {
+/* Copyright (c) 2012-2022 The ANTLR Project. All rights reserved.


it's 2023 :-)

My last information about copyrights in ANTLR4 is that we always keep the original ones and don't update any year, but maybe that only belongs to the first year, not the second one.

Once I have a definite answer from @parrt I'll update them.

ericvergnaud · 2023-09-05T14:11:41Z

runtime/JavaScript/src/antlr4/CharStreams.d.ts

+ * can be found in the LICENSE.txt file in the project root.
+ */
+
+import CharStream from "./CharStream.js";


We're not moving away from typescript style imports. Not sure what problem this is trying to solve but in my experience it's a recipe for problems.

What is a "typescript import"? This is a normal JS default import as being used in many other places in the JS runtime.

this file is a ts file, not a js file.
As such it needs to follow ts import conventions i.e. no suffix.
It makes no sense to import a js file in a d.ts file in the first place, since we're only looking to import types, not implementations. Think of the d.ts files as a set of files that need to live somewhat independently of the js ones, because the js ones get babelized, uglified and ultimately packaged - they might not exist in the distribution

OK, you have a point here. These are type definitions, not normal TS files, so they disappear in the final bundle. For normal TS files the extension is necessary, though, since files are not always bundled.

This is irrelevant here, but FYI for normal TS files, the extension is also not necessary and should be avoided, unless the imported file is actually a JS one. See for example https://github.com/hyperledger/aries-framework-javascript/blob/main/packages/core/src/agent/Agent.ts.

A link to a random framework doesn't help in this discussion. Hmm, I thought I had provided the link, but cannot find it here, so here it is again: full ESM support requires the extension in Node.js (in the browser they are required anyway). It's the bundler that automatically handles that, but when running with tools like ts-node or tsx they are required.

To be honest I don't know if the situation is really different for type definition files, but I believe that for consistency the extension should be included in every import statement, to avoid confusion.

Hence I'm not going to remove the extensions again.

The link you provide is for node i.e. javascript, not for typescript.
This is the official reference and you'll read that they don't use extensions except for side effects.

But ultimately all TS files end up as JS files, so this applies anyway. If you are against including the .js extension, why did you include them in the existing JS/TS runtime? You even use the extension in the test cases (see Test.ts.stg).

The main point here is: when using tools like ts-node to run TS directly you need the extensions or they will not find the imports. I've been through that for a number of times already.

We are wasting our time here on something which is an absolute minor aspect and I have good reasons to keep the extensions included.

@ericvergnaud The docs page you link is a little confusing, it makes it seem like the it uses a ".js" extension for side effects only but that is not the case.

In fact the typescript documentation has a section on file extensions in imports here, where they explain why the .js extension is needed to support both commonjs and esm for node when using typescript.

runtime/JavaScript/src/antlr4/CommonToken.js

ericvergnaud · 2023-09-05T14:13:50Z

runtime/JavaScript/src/antlr4/InputStream.d.ts

 export declare class InputStream {
-    constructor(data: string);
-    constructor(data: string, decodeToUnicodeCodePoints: boolean);
+    public constructor(data: string);


why does this constructor need to be public when the others don't ?

If there's no access modifier then the default is public. But making it explicit which visibility a member has is considered good coding style. In JS it doesn't really matter, as there is no public or private way (the `#member' notation is an addendum to improve this situation), but for TS it makes sense as the transpiler enforces the correct access.

"the transpiler enforces the correct access" -> you mean the type checker ? The transpiler drops this since it's not supported by JS.

ericvergnaud · 2023-09-05T14:17:56Z

runtime/JavaScript/src/antlr4/TokenStream.d.ts


 export declare class TokenStream {
+    public tokenSource: TokenSource;
+    public index: number;
+    public get size(): number;


using a getter is left to the implementation, from a contract pov it's a field

It's not. A single getter makes this member readonly (and shows there's potentially more processing than just a single field). Why hiding such information, if the language allows that?

ericvergnaud · 2023-09-05T14:19:53Z

runtime/JavaScript/src/antlr4/atn/ParserATNSimulator.d.ts


-    constructor(recog: Recognizer<Token>, atn: ATN, decisionToDFA: DFA[], sharedContextCache: PredictionContextCache);
-    adaptivePredict(input: TokenStream, decision: number, outerContext: ParserRuleContext) : number;
+    public constructor(recog: Parser, atn: ATN, decisionToDFA: DFA[], sharedContextCache: PredictionContextCache);


again, why public here ?

ericvergnaud · 2023-09-05T14:20:58Z

runtime/JavaScript/src/antlr4/atn/index.d.ts

+ * Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
+ * Use of this file is governed by the BSD 3-clause license that
+ * can be found in the LICENSE.txt file in the project root.
+ */



ts imports should not have a suffix

ericvergnaud · 2023-09-05T14:24:18Z

runtime/JavaScript/src/antlr4/tree/ParseTreeVisitor.d.ts

-    visitErrorNode(node: ErrorNode): Result;
+export declare class ParseTreeVisitor<Result> {
+    public visit(tree: ParseTree): Result;
+    public visitChildren(node: ParseTree): Result;


a node with children is necessarily a ruleNode, please revert

No, a node with children is a Tree, but that's an interface in Java and there's no real sense to separate the 3 interfaces Tree, SyntaxTree and ParseTree. The first two don't add anything useful to ParseTree hence they are combined into a single interface.

The RuleNode is another interface which only adds a single member. There's only one type which uses RuleNode, which is RuleContext, so it makes sense to remove RuleNode and directly work with RuleContext. Why keep a class/interface which is not being used?

"it makes sense" is an opinion. We just have different coding styles, and I don't change working code to satisfy coding style, otherwise I would change the code 10 times per day.

Then read "make sense" as "there's a good reason". It's not personal coding style, but something useful. You also have left out things in the current runtime that you have no use for. Btw. I did the same change in the C++ runtime in 2016. The 3 different interfaces are simply over-engineered and are useless in the context of the JS/TS runtime.

I happen to find it more useful to show that the 3 types of nodes are covered by the if/else if... which is why I wrote it like that in the first place.

Can you point me to a place in source code where you test, say, on Tree? How often do you check for ParseTree and for SyntaxTree? I cannot find any place where Tree or SyntaxTree is used in JS/TS code, except to define ParseTree.

And even in the other targets there's only very few use of Tree, namely in the Trees file (only Java, Dart and Swift) and in the Java UI tool, which is not relevant here.

I think these are enough points to warrant a simplification of this small corner of the ANTLR4 type tree. That's why I have no intention to revert that.

runtime/JavaScript/src/antlr4/tree/Trees.js

Signed-off-by: Mike Lischke <[email protected]>

Changed the test template to not use a .js file extension in imports and use RuleContext instead of RuleNode to check tree validity (because RuleNode will go). Signed-off-by: Mike Lischke <[email protected]>

mike-lischke added 3 commits September 5, 2023 14:06

Clean up of the JS/TS target

26a71a5

- Added copyright were missing. - tab -> space conversion. - General formatting. Signed-off-by: Mike Lischke <[email protected]>

An unwanted change creeped in.

ddfc852

Signed-off-by: Mike Lischke <[email protected]>

Trying to fix the tests.

ffd8ec9

Everything is good locally. Why's GH actions failing? Signed-off-by: Mike Lischke <[email protected]>

ericvergnaud requested changes Sep 5, 2023

View reviewed changes

mike-lischke added 2 commits September 6, 2023 10:24

Merge branch 'dev' into dev

a4a4ccd

Signed-off-by: Mike Lischke <[email protected]>

Testing without extensions + no RuleNode

c922b12

Changed the test template to not use a .js file extension in imports and use RuleContext instead of RuleNode to check tree validity (because RuleNode will go). Signed-off-by: Mike Lischke <[email protected]>

mike-lischke closed this Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up of the JS/TS target #4409

Clean up of the JS/TS target #4409

mike-lischke commented Sep 5, 2023 •

edited

Loading

ericvergnaud Sep 5, 2023 •

edited

Loading

mike-lischke Sep 5, 2023

mike-lischke Sep 6, 2023

ericvergnaud Sep 6, 2023

mike-lischke Sep 6, 2023

ericvergnaud Sep 5, 2023

mike-lischke Sep 5, 2023

mike-lischke Sep 6, 2023

ericvergnaud Sep 5, 2023

mike-lischke Sep 5, 2023

ericvergnaud Sep 5, 2023

mike-lischke Sep 5, 2023

ericvergnaud Sep 5, 2023

mike-lischke Sep 6, 2023 •

edited

Loading

ericvergnaud Sep 6, 2023

mike-lischke Sep 6, 2023

OskarDamkjaer Sep 20, 2023 •

edited

Loading

ericvergnaud Sep 5, 2023

mike-lischke Sep 5, 2023

ericvergnaud Sep 5, 2023

ericvergnaud Sep 5, 2023

mike-lischke Sep 5, 2023

ericvergnaud Sep 5, 2023

ericvergnaud Sep 5, 2023

ericvergnaud Sep 5, 2023

mike-lischke Sep 5, 2023 •

edited

Loading

ericvergnaud Sep 5, 2023

mike-lischke Sep 5, 2023

ericvergnaud Sep 5, 2023

mike-lischke Sep 6, 2023

		@@ -1,5 +1,9 @@
		export declare class CharStream {
		/* Copyright (c) 2012-2022 The ANTLR Project. All rights reserved.

Clean up of the JS/TS target #4409

Clean up of the JS/TS target #4409

Conversation

mike-lischke commented Sep 5, 2023 • edited Loading

ericvergnaud Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-lischke Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarDamkjaer Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-lischke Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-lischke commented Sep 5, 2023 •

edited

Loading

ericvergnaud Sep 5, 2023 •

edited

Loading

mike-lischke Sep 6, 2023 •

edited

Loading

OskarDamkjaer Sep 20, 2023 •

edited

Loading

mike-lischke Sep 5, 2023 •

edited

Loading