Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(@ngtools/webpack): show warning when a TS compilations contains … #15061

Merged
merged 1 commit into from
Jul 25, 2019
Merged

feat(@ngtools/webpack): show warning when a TS compilations contains … #15061

merged 1 commit into from
Jul 25, 2019

Conversation

alan-agius4
Copy link
Collaborator

…unused files

When a tsconfig without includes or files all files under the rootDir will be included in the compilation. This results in redundant files to inserted as part of the ts compilation which in some cases might reduce the compilation drastically.

Related to: TOOL-949

@alan-agius4
Copy link
Collaborator Author

alan-agius4 commented Jul 12, 2019

Note: tests to be added

Also without implementing this #15030 for VE the warning will be shown all the time for newly generated apps.

This is because by default we do include redundant files in the compilations. There are;

  • app.server.module.ts
  • server.ts
  • main.server.ts
  • environment.prod.ts

NB: All existing projects will have this warning.

@clydin
Copy link
Member

clydin commented Jul 12, 2019

We may want to ignore anything involved in file replacements.

@alan-agius4
Copy link
Collaborator Author

Only the last one, is in fileReplacement but, why should we ignore it? It is in the compilation because of the non strict includes that we currently have as otherwise they won’t be part of the compilation.

@alan-agius4
Copy link
Collaborator Author

As discussed we should only enable this for Ivy.

@alan-agius4 alan-agius4 added PR state: blocked and removed PR state: blocked needs: discussion On the agenda for team meeting to determine next steps labels Jul 16, 2019
@alan-agius4 alan-agius4 marked this pull request as ready for review July 17, 2019 07:54
@alan-agius4 alan-agius4 added target: major This PR is targeted for the next major release and removed state: WIP labels Jul 17, 2019
Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this ended up not requiring a lot of changes.

Most of my change requests are actually questions.

packages/ngtools/webpack/src/angular_compiler_plugin.ts Outdated Show resolved Hide resolved
return [...uniqueDependencies]
.filter(x => !!x) as string[];
// Add used source files
this._usedSourceFiles.add(forwardSlashPath(fileName));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat reluctant on adding to the list here. I agree that it's a convenient place to hook into, but it adds somewhat unrelated things to the mental model for this call.

Would this approach work as well if added to getCompiledFile instead (assuming adding imports is not needed either)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked and getCompiledFile will not be called for all files, such as type only files.

So if we want to remove this from here, what we can do is to expose the _usedSourceFiles publicly and from the loader in https://github.com/angular/angular-cli/blob/master/packages/ngtools/webpack/src/loader.ts#L77 we do:
Ex:

        [...new Set(dependencies)].forEach(dep => {
          plugin.updateChangedFileExtensions(path.extname(dep));
          this.addDependency(dep);
          plugin.usedSourceFiles.add(forwardSlashPath(fileName));
        });

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding an additional Webpack compilation hook inside the Angular plugin that compares the set of TS source files with webpack modules? Not sure of the appropriate hook off the top of my head though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s sound like a good idea I think the hook was called finishedModules or something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clydin I tried the above, but the problem is that type only files will not be part of the webpack modules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to get the information needed through buildInfo.

packages/ngtools/webpack/src/angular_compiler_plugin.ts Outdated Show resolved Hide resolved
…unused files

When a tsconfig without `includes` or `files` all files under the `rootDir` will be included in the compilation. This results in redundant files to inserted as part of the ts compilation which in some cases might reduce the compilation drastically.

Related to: TOOL-949
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants