Add browser support via babel #440

unsupervisednn · 2020-02-02T18:24:42Z

This takes a different approach from #413. Instead of modifying dozens of typescript files in the fragile hopes of getting it to compile nicely to es5, I took the es2015 generated by TS and gave it to babel to transform. This seems simpler and easier because instead of relying on buggy TS es5 support, we can use industry standard babel which will hardly ever fail at transforming es2015+ to es5. I do not think the responsibility of getting working es5 should fall on the TS programmer. I used gulp to take the output of TS and stream it to babel without the need to hit the filesystem twice while compiling.

I had tried to use rollup before (#422) but rollup is not suited to compiling individual files to es5. It is a bundler and designed to output the whole library into a single file. This breaks clients which import/require the individual files such as "antlr4ts/atn/ActionTransition". Rollup would only help in making a bundle file meant to be added directly to client HTML to reduce script requests, perhaps as a UMD file. However, most client projects will have their own bundler that statically includes all necessary library code.

BurtHarris · 2020-02-03T05:04:53Z

Thank you for the pull request. I'm looking forward to reviewing it.

You touch on a point I think may have contributed to the stalling of this project. We did work a few years ago that should (if implemented correctly) eliminate the need for import statements that delve into the directory structure like

import { ActionTransition } from "antlr4ts/atn/ActionTransition";

can and should be eliminated. The Façade Pattern implemented in the index.ts files means that

import { ActionTransition } from "antlr4ts";

should work, and is less fragile. Of course, as you correctly state, that would be a breaking change, but that's what major versions in semantic versioning are for. We're well overdue for a V1 release, and I think some cleanup of the way clients reach into the run-time code is would be appropriate. Bundlers like rollup and webpack are an important part of the modern javascript ecosystem, but neither @sharwell nor I had any experience with them when we started the port.

BurtHarris · 2020-02-03T05:13:17Z

Do you have any specific reason to say that Typescript's ES5 support is buggy?

unsupervisednn · 2020-02-03T08:42:06Z

The Façade Pattern implemented in the index.ts files means that import { ActionTransition } from "antlr4ts"; should work, and is less fragile.

On the one hand it would be better to be able to rearrange library directory structure without impacting clients in future versions, on the other it forces clients to have to have a tree shaking bundler if they want to minimize their browser bundles. It is not my call to make.

Do you have any specific reason to say that Typescript's ES5 support is buggy?

If you enable these options in tsconfig

    "lib": [
      "ES2017",
      "ES2016",
      "ES2015",
    ],
    "downlevelIteration": true,
    "target": "ES5",

you will quickly run into problems #413 was facing, first of which is:

error TS2340: Only public and protected methods of the base class are accessible via the 'super' keyword

Looking at TS's issue you will see that they don't really care to support this compilation to ES5. That is a specific example but in general it is very telling of their support for ES5 compilation. Even though babel can compile the code just fine from ES6 to ES5, Typescript forces you to think about ES5 compilation quirks even with valid Typescript. If TS only fully supports compilation to ES6 then we should use babel to compile to ES5. There are many more of these sorts of problems that were addressed in #413.

One perk of using babel is that with @babel/preset-env you can choose the coverage of browsers to support in ES5 via a browserlist query

BurtHarris

Thanks for the PR. I've got some questions.

BurtHarris · 2020-04-10T19:58:18Z

.nvmrc

@@ -0,0 +1 @@
+10.12.0


Our convention is to always have a newline at the end of all files. The file '.editorconfig' generally fixes this, but your editor may not be using it. There are plugins, e.g. I use an editorconfig add-in for vscode.

Can you explain the effect of adding this .nvmrc file? Does it work with the original nvm, nvm-windows, or both?

BurtHarris · 2020-04-10T19:58:58Z

gulpfile.js

+    .pipe(gulp.dest('target/src'));
+};
+
+exports.default = build;


newline at end

BurtHarris · 2020-04-10T20:06:57Z

gulpfile.js

+function build() {
+  return gulp.src('src/**/*.ts')
+    .pipe(sourcemaps.init())
+    .pipe(tsProj())


I've seen projects where they translate the .ts directly into .js using babel with @babel/preset-typescript @babel/preset-env @babel/plugin-proposal-class-properties @babel/plugin-proposal-object-rest-spread. That approach is discussed in a blog entry from the typescript team. The tsc command in that approach is only used for error checking.

The approach you have here seems to run tsc and babel differently with the tsc output feeding into babel. Can you explain why you chose that approach?

Webpack has the same problem as rollup: it was mainly designed to bundle JS into one file. Good for shipping to browser only for HTTP1 concerns, but for libraries it is best to ship in multiple formats and let the user decide how they would like to consume it. Gulp is the easiest way to allow that.

Let me know if you would like me to keep updating this PR.

Ah I see your question was not about webpack (which recommends uses this method for compiling TS) specifically. I still standby my point that it is better to ship in multiple formats, which gulp is better designed for.

BurtHarris · 2020-04-10T20:08:59Z

tool/pom.xml

@@ -185,8 +185,8 @@ Licensed under the BSD-3-Clause license. See LICENSE file in the project root fo
                    <execution>
                        <id>default-compile</id>
                        <configuration>
-                            <source>1.6</source>
-                            <target>1.6</target>
+                            <source>1.8</source>


@sharwell, are you OK with this change in compiler version required?

BurtHarris · 2020-04-10T23:37:02Z

tsconfig.json

@@ -7,6 +7,8 @@
    "lib": [
      "es6"
    ],
+    "inlineSourceMap": true,


As stands, there is already a "sourceMap" option specified at line 5. This "inlineSourceMap" at line 11 conflicts with the earlier one it and generates a compile error on my machine. I suggest you bring in the latest tsconfig.json from the project master branch.

stevengum · 2020-06-07T22:44:49Z

@unsupervisednn @BurtHarris, it looks like the build is failing because the CI uses Node.js v6.

Specifically, gulp-typescript relies on source-map which requires Node.js 8+. (gulp-typescript#584, permalink to engines)

The README.md points to supporting Node.js 6.7.x.

AppVeyor step that downgraded to v6
First ReferenceError regarding WebAssembly

alessiostalla · 2021-01-04T14:04:07Z

What's blocking this from being merged?

alexkreidler · 2022-03-23T18:03:14Z

Any updates on this?

I took a shot at running this code in the browser, but unfortunately ran into errors with various Nodejs modules that are needed, like util and assert. There are polyfills for some of them, but they are different for eg Webpack and Vite, so it might be good to move away from them overall.

jakeanstey · 2023-12-13T15:49:36Z

Using Rollup I was able to get this to work in the browser, shimming util and assert. Would be nice to have this merged in.

Change minimum java to 1.8 to fix maven

19e0a57

unsupervisednn force-pushed the master branch from bfbe71b to ee2aec9 Compare February 2, 2020 18:30

Use babel and gulp to compile to es5

900720a

unsupervisednn force-pushed the master branch from ee2aec9 to 900720a Compare February 2, 2020 18:31

BurtHarris reviewed Apr 10, 2020

View reviewed changes

stevengum mentioned this pull request Jun 8, 2020

[Adaptive-Expressions] Pick up updated antlr4ts package with ES5 support microsoft/botbuilder-js#2313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add browser support via babel #440

Add browser support via babel #440

unsupervisednn commented Feb 2, 2020

BurtHarris commented Feb 3, 2020

BurtHarris commented Feb 3, 2020

unsupervisednn commented Feb 3, 2020 •

edited

Loading

BurtHarris left a comment

BurtHarris Apr 10, 2020

BurtHarris Apr 10, 2020

BurtHarris Apr 10, 2020

unsupervisednn Jun 1, 2020

unsupervisednn Jun 1, 2020 •

edited

Loading

BurtHarris Apr 10, 2020 •

edited

Loading

BurtHarris Apr 10, 2020 •

edited

Loading

stevengum commented Jun 7, 2020

alessiostalla commented Jan 4, 2021

alexkreidler commented Mar 23, 2022

jakeanstey commented Dec 13, 2023

		@@ -0,0 +1 @@
		10.12.0

Add browser support via babel #440

Are you sure you want to change the base?

Add browser support via babel #440

Conversation

unsupervisednn commented Feb 2, 2020

BurtHarris commented Feb 3, 2020

BurtHarris commented Feb 3, 2020

unsupervisednn commented Feb 3, 2020 • edited Loading

BurtHarris left a comment

Choose a reason for hiding this comment

BurtHarris Apr 10, 2020

Choose a reason for hiding this comment

BurtHarris Apr 10, 2020

Choose a reason for hiding this comment

BurtHarris Apr 10, 2020

Choose a reason for hiding this comment

unsupervisednn Jun 1, 2020

Choose a reason for hiding this comment

unsupervisednn Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

BurtHarris Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

BurtHarris Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

stevengum commented Jun 7, 2020

alessiostalla commented Jan 4, 2021

alexkreidler commented Mar 23, 2022

jakeanstey commented Dec 13, 2023

unsupervisednn commented Feb 3, 2020 •

edited

Loading

unsupervisednn Jun 1, 2020 •

edited

Loading

BurtHarris Apr 10, 2020 •

edited

Loading

BurtHarris Apr 10, 2020 •

edited

Loading