antlr4ts icon indicating copy to clipboard operation
antlr4ts copied to clipboard

Add browser support via babel

Open unsupervisednn opened this issue 5 years ago • 7 comments
trafficstars

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.

unsupervisednn avatar Feb 02 '20 18:02 unsupervisednn

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 avatar Feb 03 '20 05:02 BurtHarris

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

BurtHarris avatar Feb 03 '20 05:02 BurtHarris

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

unsupervisednn avatar Feb 03 '20 08:02 unsupervisednn

@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

stevengum avatar Jun 07 '20 22:06 stevengum

What's blocking this from being merged?

alessiostalla avatar Jan 04 '21 14:01 alessiostalla

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.

alexkreidler avatar Mar 23 '22 18:03 alexkreidler

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.

jakeanstey avatar Dec 13 '23 15:12 jakeanstey