Ordering issue between @ngtools/webpack + other loader

[imirkin]

🐞 bug report

Affected Package

@ngtools/webpack

Is this a regression?

This worked fine with 8.1.2 but behaves oddly with 9.0.0-rc.7.

Description

First of all - I'm hardly a webpack expert. What I want is simple - remove a bunch of "this.logger.info" statements from the production build. Try as I might to convince uglify-js & co to do this, I failed (this was all so easy in closure-compiler... sigh). So the solution was to use string-replace-loader to just replace those statements with 0's before anything else happened. While this is not perfect, it works for us.

However starting with Angular 9 (with Ivy enabled, in case it matters), this no longer works as before. I have to flip the order of the loaders, and then it works fine again. I believe this points to a deeper issue though, hence this report.

🔬 Minimal Reproduction

In the prod build (which inherits some common thing), we have:

commonConfig.module.rules.push({ test: /\.ts$/, use: [
  {
    loader: '@ngtools/webpack'
  },
  {
    loader: 'string-replace-loader',
    options: {
      search: 'this\\.logger\\.(trace|debug|info)\\s*\\([^()]*\\)\s*;',
      replace: '0;',
      flags: 'sg',
    }
  }
] })

This works fine with Angular 8.1.2, but I have to flip the order of those two loaders for Angular 9.0.0.rc-7. If I flip the order with Angular 8, I end up with a bunch of _0's, because there's some transform that has already converted this -> _this (presumably tsc, invoked by @ngtools/webpack). However in Angular 9.0.0.rc-7, the string-replace-loader ends up doing nothing, all the logs are in the resulting output. Flipping the order to make it come after @ngtools/webpack, however, causes it to get rid of the logger.info's as expected.

I would have expected the original thing to be correct, based on my understanding of loader chaining.

[AndrewKushnir]

Hi @imirkin, this question seems to be related to Angular CLI, so I'm transferring it to CLI repo. Thank you.

[clydin]

The @ngtools/webpack loader does not fully support loader chaining. All loaders executed prior to the loader will be ignored. This behavior is intentional to fully support TypeScript and Angular AOT compilation which requires type information. TypeScript projects are compiled as a whole and not on an individual and isolated file basis. The later of which being the webpack loader model.

Loaders executed after the @ngtools/webpack loader will, however, function as intended. The loader outputs javascript (and sourcemaps if enabled).

It is actually surprising that this setup using only the rule shown above actually worked on either version. As the loader completely ignores any input code from prior loaders (code reference).

[imirkin]

Well, I guess ignorance is bliss -- it does work the way I posted with 8.1.2, and flipping their order with 9.0.0.rc-7. I don't have a great mental model for how this stuff works in webpack, so I just went for it... and the resulting .js files are the proof.

So ... is there any alternate advice for making this work in a more proper way?

Note that loader chaining shouldn't prevent having type information -- it's still typescript on the output, just a transformed version from a VFS. Or at least that's how my mental model of this goes.

[clydin]

The loaders are essentially a chained set of function calls. Each of them being called with the return values of the previous.
Since using loaders prior to the @ngtools/webpack loader is not intended to be supported, this not really a defect. However, using a loader to modify the javascript after the loader should not be a problem as the loader is required to output javascript to allow webpack to properly function.

As an alternative, you could consider replacing the logger implementation file with a noop version for production builds. If using webpack directly, an alias configuration should allow that to work.
Or if using dependency injection, a different instance of the logger could be provided when initializing the application based on its build configuration.

As an aside, the string replace loader also appears to not process sourcemaps when it changes the code. Unless the replacement is the exact same character count, the sourcemaps will be broken.

[imirkin]

Hm OK. The sourcemap thing might be an issue - didn't test that yet, thanks for the heads-up.

The situation is that we have a logger which can optionally send messages back to the server, and in production builds, we only want to do it for errors. It's an injected logger instance, not a global, and I've had a really hard time getting rid of the logs at the call site. Basically the code does a bunch of

this.logger.info("asdf")

And while I can provide an alternate logger impl or otherwise indicate that the .info() call does nothing, I want to get rid of that whole line of code at the call site (the long string, and potentially dead variables referenced by the info message). That's the bit I've had trouble with. It's easy enough with closure-compiler -- you can tell it to just drop certain methods, but without the advanced type analysis, I can see how uglify has trouble.

Anyways, thanks for looking at this, too bad that advanced loading techniques for ts files aren't supported. (I don't really expect you to solve my problems, but if you have advice on how to drop the call site calls, it'd be much appreciated.)

[clydin]

From a performance perspective, there shouldn't be much difference for the noop call. Assuming there is not any performance heavy code within the arguments of the call.

If removing the calls is a priority, then some additional structure can be added around each of the development logger calls to allow them to be easily removed by the optimizer. Something similar to the following, for instance:

if (typeof appDisableDevLogging === 'undefined') {
  this.logger.info('abc');
}

Then in the terser configuration for production builds, you can use the global_defs option to define appDisableDevLogging to true (or technically anything other than undefined). The typeof is important here since in strict mode, which is the default for module scripts (ES2015), you'll get a reference exception by directly accessing it if it doesn't exist.

[imirkin]

Right. I was aware of the per-callsite thing, but this is throughout. Too bad. With closure-compiler, there was a way to globally remove all goog.debug.* calls.

The issue isn't perf -- it's size, and potentially disclosure of information. Thanks for the advice!

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}