"ng build" not removing unneccessary code

[angelaki]

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

No

Description

When creating a new project via Angular (v14) CLI ng new test, paste the following code in any component and check the deployment's output

constructor() {
  //#1 sure stays always
  console.log('THIS SHOULD HAPPEN');

  //#2 gets always removed
  if (false) { console.log('THIS SHOULDNT HAPPEN'); }

  //#3 doesn't get removed in my complex solution but only in a clean one
  if (!environment.production) { console.log('THIS SHOULDNT HAPPEN TOO'); }
}

#1 & #2 are removed from the code es expected. In my pretty complex solution (mixins, inheritences etc.) only #2 gets removed - the !environment.production stays in the output! Can someone please help me find the reason for this? I've already tried pretty much but just can't figure it out.

I asked for this on Stackoverflow (https://stackoverflow.com/questions/73318053/angular-14-ng-build-not-removing-unneccessary-code) already and was told to activate the old flags in the angular.json but, as expected, this didn't work, too. Since I have absolutely no idea what could cause this, I consider it to be a bug (?). Or is this maybe Typescript related and I'd need to ask there?

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in (run ng version)

Angular CLI: 14.0.0
Node: 16.13.2
Package Manager: npm 8.5.4

Anything else?

No response

[alan-agius4]

This is expected since the minifier is unable to statically analyse environment.production and determine it's value when the environment file is imported and referenced cross-chunk.

The will be addressed in the future with the introduction of tokenization that will be eventually replace file replacements.

Closing since this is working as expected.

[angelaki]

(When) is this likely to happen? I was expecting these code lines being removed in my output. It's pretty annoying to have that much debug code in my prod release.

Edit: oh... it just got not planned. Any alternatives for this? I guess it is pretty likely that the environment-file get imported cross chunks. And keeping all this debug code is just ugly.

[alan-agius4]

Thinking a bit more about this, let me mark this for further discussions with the team. As currently we already have a way to use "substitutions" ngtools however this is not yet exposed in build-angular.

[angelaki]

In a different project I just noticed that (as expected) code won't even get removed for different providers registered. In a scenario, where you want to register different services for your types (e.g. https://blog.10pines.com/2017/12/01/substitution-based-on-environments-and-di-in-angular/, just googled some random tutorial), I'd expect only the chosen code for production being in the output. Right now all the code gets bundled. That's a huge impact on the application size!

And I guess choosing between different implementations for repositories via environment is a pretty common use-case!

[dgp1130]

Discussed this in detail today and there are quite a few nuances here.

The root issue here is that Webpack chunks the application before it is fed into Terser to optimize. Webpack also uses a customized lazy-loading scheme (it's not just a dynamic ESM import), which Terser doesn't understand. As a result, Terser can't effectively follow the dependency at build-time and be confident that it is a constant eligible for tree shaking.

There are a few potential options here. The first one is to make ngDevMode public API and your code can be re-written to if (ngDevMode) { /* ... */ }. This value is effectively substituted by the build process so Terser will resolve it as a constant true or false and can tree shake accordingly. We've been thinking about this for a while, but it hasn't happened yet.

A second approach is to provide one more level of flexibility and allow custom substitutions, so users can specify logFromMyConstructor = true in some configuration file and be able to write if (logFromMyConstructor) { /* ... */ }. While a nice feature, our main concern is that when and where to use substitutions and its implications on tree shaking are very nuanced and hard to communicate and can be very easily misused. We'd also need to figure out typing since somewhere we need to include:

declare global {
  var logFromMyConstructor: boolean;
}

It's not hard to include that in the project, but we want to make sure it plays well with the TypeScript language service, so this type definition needs to be statically analyzable.

This approach also begs the question of whether this substitution is done before or after the Angular compiler runs, which effectively asks the question "Should we allow substitutions to apply to Angular decorators?" For example, would it be valid to write:

@Component({
    // Conditionalize compile-time information (what template to use) on build substitution.
    template: useCoolTemplate ? 'cool.html' : 'less_cool.html',
})
class MyComponent { }

I believe you can do this today with environments as the Angular compiler will resolve such expression at compile time. I'm not sure that's a pattern we want to encourage or propagate though.

A third option is to lean in to ESM formatting (particularly with the experimental ESBuild-er) and possibly get something more tree shakable. Since that uses native dynamic imports for chunking, it should be viable to statically analyze cross imports and identify compile-time constants. The challenge with this is that it's often hard to prove that environments.production is truly constant, since any kind of foo[key] = {} statement might modify it if foo === environments and key === 'production', which can't be disproven statically. This might be more viable if we updated environments to directly export their data instead of using an object.

// environments.ts
// Directly export values.
export const production = false;

// usage.ts
const { production } = await import('./chunk-abc123.js');
if (production) {
  // Definitely constant and tree shakable.
}

I'm not sure if ESBuild can actually do this today, but it seems like something it could feasibly do.

Fourth, we could do more of a redesign of environments to not require file replacements in the first place. We're not 100% on that would look like, but it could be something that's implemented as a build-time substitution or injected into the application semi-automatically.

Tangentially, I'm also thinking through whether or not we need environments to tree shake if we can provide the appropriate other tools like ngDevMode. Could diverging the concepts of "tree shake debug code" and "application configuration" still meet most user needs? These are just high-level thoughts, we need more investigation and probably an RFC to know if such a change would still meet the community's needs.

Those are all big questions and ideas, but I think the most actionable part for this particular issue to work towards exposing ngDevMode as a public API. That should be easiest solution to land and would probably be sufficient for most use cases. If we still find users wanting to tree shake on environment data which doesn't easily map to ngDevMode, we can explore some of those other solutions and see what the right approach is.

[pburgmer]

Thanks for the detailed explanation @dgp1130. Please keep library authors in mind. I would like to get rid of some code in my library, if the application is compiled in prod mode. Environment replacement doesn't work for libraries as far as I know. I would love to see ngDevMode as public API. Would it be possible to define it as a const, which can be imported from an EcmaScript module, instead of a global variable?

[k3nsei]

@alan-agius4 @pkozlowski-opensource I'm an author of Angular Library and I want to have strict check on dev mode, but those should not end up in prod bundle. As you know libraries don't have access to anything other than isDevMode. It's almost 2 years and no progress on that one. Please prioritize it as critical regression!

[JeanMeche]

Also, for the sake of linking issues, the related issue on the FW repo is angular/angular#51175.