mike-lischke / antlr4ng

Next Generation TypeScript runtime for ANTLR4
Other
85 stars 15 forks source link

Exception in production mode of webpack #38

Closed HaydenOrz closed 7 months ago

HaydenOrz commented 7 months ago

Description

In Webpack's production mode, if the optimization.minimize option is turned on (Webpack has this option turned on by default), will get an error.

image

image

It looks like it might be an issue with the compression tool, but I feel the need to report this issue to you, because webpack is very widely used.

Minimal demo to reproducing the problem

https://github.com/HaydenOrz/antlr4ng-trino


The good news is that Antlr4ng performs well in all of these situations:

mike-lischke commented 7 months ago

Usually, the error that a type is not defined when running a TS/JS app, means that it wasn't imported yet (because of circular dependencies). However, when bundled, such an error should never come up, since everything is in one file. Unless of course, you have separated certain parts into own bundles. I do that with vite using the rollup option manualChunks and have seen this error in certain situations.

HaydenOrz commented 7 months ago

I do that with vite using the rollup option manualChunks and have seen this error in certain situations.

I made a similar attempt, but I didn't find this issue in the vite project. This issue doesn't seem to be related to chunk distribution. Currently I'm only reproducing this issue in production mode for webpack. And when I turned off webpack's optimization.minimize option, this issue no longer appears. In addition, when Webpack builds an application, code compression occurs after chunk distribution.

So for now, I'm simply thinking that the issue is only due to webpack's built-in code compression plugin. With this as a premise, I made some attempts. I noticed that antlr4ng uses esbuild to build the release package, and the minify option is turned on at build time. And by default, webpack uses the TerserWebpackPlugin to compress the code. And it supports the use of esbuild for compression.

Then, I did the following in the demo mentioned above:

Installed dependencies

npm install esbuild terser-webpack-plugin -D

Modified webpack.prod.config.js

const devConfig = require('./webpack.config');
const TerserPlugin = require('terser-webpack-plugin');

delete devConfig.devServer;
delete devConfig.devtool;

// devConfig.devtool = 'source-map'

devConfig.mode = 'production';
devConfig.optimization = {
    minimizer: [
        new TerserPlugin({
            minify: TerserPlugin.esbuildMinify,
        }),
    ],
};

module.exports = devConfig;

Then the problem no longer appears.

HaydenOrz commented 7 months ago

I'm not familiar with code compression, but it looks like it's the result of a conflict between code compression plugins.

Also, I think antlr4ng might fix this by not turning on the minify option at build time. This requires some additional testing.

mike-lischke commented 7 months ago

Interesting, I didn't know that. Actually, I was thinking the other day if I should keep the minify option on, given that it doesn't make things really faster, but gets in the way when debugging test applications. So, maybe I should switch off minification...

HaydenOrz commented 7 months ago

Interesting, I didn't know that. Actually, I was thinking the other day if I should keep the minify option on, given that it doesn't make things really faster, but gets in the way when debugging test applications. So, maybe I should switch off minification...

Haha, these coincidences help you make your choice.

Also, in my opinion, a library doesn't need to be compressed, especially for the JS ecosystem, since almost all projects will have bundlers. Code compression for libraries generally only appears on CDN resources.

HaydenOrz commented 7 months ago

I did some trying, turned off minify for antlr4ng, and tested it in the webpack project, and the results were frustrating.

The process is as follows:

Run following commands in antlr4ng:

npm run build

npm run pack

Then I got a tarball of antlr4ng, named antlr4ng-2.0.10.tgz.

Move it to the root directory of the demo project above and run the following command in the demo project:

npm install ./antlr4ng-2.0.10.tgz

Then I build the demo project and preview it, but I still get the following error:

image

HaydenOrz commented 7 months ago

Maybe it's the same issue with https://github.com/mike-lischke/antlr4ng/issues/34 and https://github.com/mike-lischke/antlr4ng/issues/31

HaydenOrz commented 7 months ago

I made another attempt to abandon esbuild to bundle when antlr4ng build, and only compile with tsc. Then I found that antlr4ng built like this worked well in the webpack project without any issues.

I believe the problem may be with webpack or terser, but at the moment I haven't found a good way to get antlr4ng to work with webpack and terser.

If you have any progress on this issue, please let me know, appreciated.

HaydenOrz commented 7 months ago

Hi Mike, here's good news!

I've found that turning on esbuild's --keep-names option solves this problem. This requires modifying antlr4ng's build-bundle command to

esbuild ./src/index.js --main-fields=module,main --bundle --target=esnext --keep-names

antlr4-c3 has the same problem and can be solved in the same way.

I've done quite a bit of testing in both webpack and vite projects, both in production mode and in development mode.

Enabling this option will result in a different packaging result than before it was enabled, as shown below:

  1. Two new lines of code have been added to the file header, which are used to keep the name of the class or function

    var __defProp = Object.defineProperty;
    var __name = (target, value) => __defProp(target, "name", { value, configurable: true });
  2. A static methods will be added to all classes(in the case of ParseTreeWalker):

    var ParseTreeWalker = class _ParseTreeWalker {
      static {
        __name(this, "ParseTreeWalker");
      }
      // ...
     }

If you accept the above changes, I'm willing to create a PR. In addition, I can provide a demo repository with all my test cases if you need it.

mike-lischke commented 7 months ago

Hey @HaydenOrz, thanks for the PR. Hopefully this has no impact on the execution speed.

HaydenOrz commented 7 months ago

Hey @HaydenOrz, thanks for the PR. Hopefully this has no impact on the execution speed.

These new static blocks of code will only be executed when the class is initialized, i.e., only once. So I don't think this will have any impact on the execution speed of antlr4ng.

Also, I'd like to ask, do you have any plans to release a new version in the near future, I need this change to make anltr4ng integrate smoothly in my library. The same goes for antlr4-c3.

mike-lischke commented 7 months ago

If it's urgent I can try to set out new releases today.

HaydenOrz commented 7 months ago

If it's urgent I can try to set out new releases today.

That couldn't be better

I need to finish switching at runtime within this week.

Also, I created the same PR in antlr4-c3 and solved the same issue. I hope that antlr4-c3 can be upgraded to a new version of antlr4ng and that antlr4-c3 will also be released.