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

Improve build for indirectly affected entry points in watch mode #2734

Open
Carlosamouco opened this issue Nov 30, 2023 · 9 comments
Open

Improve build for indirectly affected entry points in watch mode #2734

Carlosamouco opened this issue Nov 30, 2023 · 9 comments

Comments

@Carlosamouco
Copy link

Carlosamouco commented Nov 30, 2023

Type of Issue

[ ] Bug Report
[X] Feature Request

Description

Whenever I am building a library in watch mode, all the indirectly affected entry points relative to the entry point that I am working also get rebuilt. Even though this is a good thing, as it detects if we have caused any side effects on other dependent entry points, it also makes rebuilds slow when there are a lot of entry points dependent on that changed entry point.

After inspecting the build pipeline, I got the impression that, despite it being smart about what needs to be recompiled when rebuilding indirectly affected entry points, some steps could be skipped as they don't warrant any extra safety and hurt application rebuilds if we are running "ng serve" alongside the watch command for our library.

An example of this, is the updates to the version of the package.json whenever an entry point is rebuild, but not just that, as any unnecessary rewrites to the /dist folder should be avoided as they trigger app rebuilds for no reason.

How To Reproduce

N.A.

Expected Behaviour

When in watch mode, file writes to the dist folder should only be made strictly when necessary, to avoid triggering application rebuilds when we are running a development server and rebuilding a library in watch mode.

Also, It would be cool to have a setting in the ng-package.json of a given entry point to unwatch external dependencies. Then, we could postpone type checking to whenever we were ready to do so, by triggering a full rebuild or by saving any file from an entry point that we want to type check. This would be really useful when we have multiple dependent entry points and type checking all of them for any given change takes a long time and is often unnecessary.

Version Information

N.A.

@alan-agius4
Copy link
Member

alan-agius4 commented Dec 1, 2023

Hi @Carlosamouco,

Thanks for the issue I'll try to answer your all your comments and questions.

it also makes rebuilds slow when there are a lot of entry points dependent on that changed entry point.

I am curious to know which part of the incremental build steps is slow. An incremental build of an entry-point is composed of multiple steps, File Analysis, TypeScript TypeChecking, Stylesheet processing, TypeScript File Writes, Generating FESM if needed and Writing assets. So it would be helpful to provide a CPU profile or a minimal reproduction so that we can investigate and potentially reduce the bottlenecks. It would also be helpful to provide the output of ng version.

An example of this, is the updates to the version of the package.json whenever an entry point is rebuild

This is intended by design and is needed for webpack to pickup changes in libraries. This is because webpack uses the version in package.json to invalidate all the entry-points of the library and not the actual JS content when caching is enabled. By the way, are you using webpack or esbuild?

Unnecessary rewrites to the /dist folder should be avoided as they trigger app rebuilds for no reason.

We do indeed re-emit the files of effected entry-points during an incremental build, for a number of reasons.

  • This is how TypeScript work, were during an incremental build it will re-emit all the files.
  • It's also the faster thing to do.

We could potentially keep track to see if the any file changed, before emitted it and updating the version in the package.json of the library. This could be done if there is a strong interest from the community for this. That said, I would expect that most of the file saves in a library do change either the JS or DTS file.

Also, It would be cool to have a setting in the ng-package.json of a given entry point to unwatch external dependencies. Then, we could postpone type checking to whenever we were ready to do so, by triggering a full rebuild or by saving any file from an entry point that we want to type check. This would be really useful when we have multiple dependent entry points and type checking all of them for any given change takes a long time and is often unnecessary.

Unless you already have "skipLibCheck" : true in your library tsconfig, you can use this option to reduce TypeChecking time. See: https://www.typescriptlang.org/tsconfig#skipLibCheck

@JoostK
Copy link
Member

JoostK commented Dec 1, 2023

Note that skipLibCheck: false does not result in fully type-checking the library code, only its declaration files.

Furthermore, changes in entry-points may actually affect the emit of downstream entry-points, e.g. when Angular metadata changes or when const enums are changed. Just performing type-checking of downstream entry-points is therefore not guaranteed to be sufficient.

@Carlosamouco
Copy link
Author

Carlosamouco commented Dec 1, 2023

I am curious to know which part of the incremental build steps is slow.

Sorry, I think I didn't explain myself very well. I wasn't criticizing the time a entry point takes to build. I was referring to how slow application rebuilds get the more entry points we are rebuilding, due to the constant package.json version updates done by each entry point that trick the application to also rebuild for no reason.

We could potentially keep track to see if the any file changed, before emitted it and updating the version in the package.json of the library.

Yes, it would be amazing if this could be avoided, but also I think we should look at other things like skip copying unchanged assets. Due to this, the more entry points we have rebuilding, the more time it takes to test the change in the development server. This discourages us from better splitting the library code into multiple entry points because, as the library grows, you can just imagine that this gets worse and worse. If we could avoid doing this when their is no emitted code, we could have the application finish rebuilding much faster.

Unless you already have "skipLibCheck" : false in your tsconfig, you can use this option to reduce TypeChecking time. See: https://www.typescriptlang.org/tsconfig#skipLibCheck

I was thinking more on having a way to control the rebuild behaviour caused by a change in a dependency. I wasn't aware of the scenarios like const enums that can trigger changes in the consumer libraries emitted code. That can be a problem if we choose not to rebuild the affected entry points or other dependent libraries straight away. Non the less, I think no one will be changing const enums all the time or making breaking changes in the public API to justify the need for constant rebuilds and type checking. It would be nice during development to configure entry points that should unwatch external changes in order to avoid getting flooded with rebuilds both in the library and in the application any time we save a file.
Currently, this is possible to do at a library level but not at a entry point level. For example if we have two libraries, where Library A is a dependency of Library B, and I am working in Library A, I can choose to only build library B when I feel I am ready to do so.

Tracking the emitted files and only update the package.json when there is an actual update, would help massively when we are running a dev server at the same time. Also if we had more control over what should be rebuilt from side effects in watch mode with a config, it would further improve the development experience of creating libraries.

@alan-agius4
Copy link
Member

Sorry, I think I didn't explain myself very well. I wasn't criticizing the time a entry point takes to build. I was referring to how slow application rebuilds get the more entry points we are rebuilding, due to the constant package.json version updates done by each entry point that trick the application to also rebuild for no reason.

It remains uncertain to me whether you're utilising esbuild or webpack to build the application, please provide this information as this is crucial. The constant package.json, is required for webpack as otherwise it library doesn't get invalidated. That said, this was intended for users using npm link whilst we should support this case, we should provide a better DX for users consuming the library in same workspace/repo.

Yes, it would be amazing if this could be avoided, but also I think we should look at other things like skip copying unchanged assets. Due to this, the more entry points we have rebuilding, the more time it takes to test the change in the development server. This discourages us from better splitting the library code into multiple entry points because, as the library grows, you can just imagine that this gets worse and worse. If we could avoid doing this when their is no emitted code, we could have the application finish rebuilding much faster.

It's important to highlight that users might sometimes mishandle the creation of secondary entry points, making them overly specific. For instance, if feature-1 and feature-2 are consistently used together, creating separate entry points for each is strongly discouraged as it adds unnecessary overhead. These secondary entry points are intended to optimize code splitting, especially in webpack, reducing the inclusion of redundant code.

Could you provide insight into how many entry points your library currently has?

It would be nice during development to configure entry points that should unwatch external changes in order to avoid getting flooded with rebuilds both in the library and in the application any time we save a file.

While this might seem beneficial, it's essentially a footgun option that contradicts our design principles. Consequently, it's unlikely that such an option will ever be incorporated.

@alan-agius4
Copy link
Member

Update: it appears that updating the version is no longer required for webpack. Thus, we can remove the logic to generate a new version on every change.

PR: #2740

@Carlosamouco
Copy link
Author

Carlosamouco commented Dec 5, 2023

It remains uncertain to me whether you're utilising esbuild or webpack to build the application, please provide this information as this is crucial.

We are currently on version 15.2 using the webpack builder and planning in start using the latest version and the new builder early January.

That said, this was intended for users using npm link whilst we should support this case, we should provide a better DX for users consuming the library in same workspace/repo.

We are indeed using npm link. We have two sets of libraries split across two repositories. The "generic" libraries that export components meant to be used in any kind of application, and the "business" libraries that export components used in the final application to meet user requirements. Each library can depend on multiple other libraries.

Could you provide insight into how many entry points your library currently has?

Currently we have manly two entry points on each library. They split the library in two parts, one to be loaded upfront and the other containing different UI's that get lazy loaded.

We wanted to create many more entry points in each library to further improve the app load time. We wanted to do something similar that what is being done for example in the @angular/material package, where each independent component is exported in a different sub entry point, but we reached the conclusion that it would take much more time to recompile everything (library + app), making the development process too slow and because of that we opted to have the bare minimum of entry points possible.

You see, when we are working on a feature, we set our application server (ng serve), then we set our library in watch mode (ng build my-lib --watch). Now, imagine if we need to update some service from the main entry point that contains utilities shared across multiple UI's. We end up having to recompile every dependent entry point and the main application over and over for every change and most of the times that's completely unnecessary since breaking changes are very rare. It becomes so much time consuming, that it's unbearable to work with. We are talking of a couple of minutes minimum for any small change which is unnaceptable. Now, if we pair this with another library that is a dependency of the first, running the watch for both at the same time, it becomes a complete nightmare. Fortunately, at least we can stop watching one library and re-run the builder for the other later on.

What I dream about, is firstly better split each independent component or UI inside the library to achieve more optimized chunks when building the app, and secondly, being able to only build the library part that I am working on would make it so much faster to develop and test.

While this might seem beneficial, it's essentially a footgun option that contradicts our design principles. Consequently, it's unlikely that such an option will ever be incorporated.

I can understand the need to rebuild all the affected parts as we discussed previously to avoid any unexpected errors, but in our case where we have lots of libraries that are inter-dependent, we are never going to run like 20 or more watch commands simultaneously on each library to ensure that. That is up to our CI/PR builds. We just wanted to bring the same level of control when working on a library to avoid all the set backs that we are facing currently. Also the watch command is meant to be used during development only, so we could be less restrict on it.

It would be so cool if this was like an advanced feature of the watch mode. You have no idea how much this would improve our development experience and the overall quality of the final application, since a we could finely split the code in the way that we want without compromising development time and all the unused UIs could finally be tree shaken and lazy loaded properly.

I was looking at the code, and I think all we needed was something like this:

package.transform.ts:161

const isDescendantPath = (basePath: string, filename: string) => !path.relative(basePath, filename).startsWith('..');

for (const entryPoint of graph.filter(isEntryPoint)) {
  const { watchDependencies, basePath } = entryPoint.data.entryPoint;
  const isDirty = [...allNodesToClean].some(dependent => entryPoint.dependents.has(dependent));
  if (isDirty) {
    if (watchDependencies || uriToClean.some(url => isDescendantPath(basePath, fileUrlPath(url)))) {
      entryPoint.state = STATE_DIRTY;
    }

    uriToClean.forEach(url => {
      entryPoint.cache.analysesSourcesFileCache.delete(fileUrlPath(url));
    });
  }
}

Where watchDependencies is a config of the entry point coming from the ng-package.json with default value true.

Can you consider adding this please? 🙏

@Carlosamouco
Copy link
Author

I forgot to thank you for the mentioned PR, that should help allot. Many thanks @alan-agius4 .

@Carlosamouco
Copy link
Author

Carlosamouco commented Dec 5, 2023

I would also like to add that, obviously minimizing the application rebuilds will greatly reduce the frustration for any user that chooses to keep with the default behaviour of the watch command and not need at all to have that config. Non the less, both options would be amazing.

@xidedix
Copy link

xidedix commented Jan 22, 2024

@alan-agius4 it seems PR #2740 removed version stamping and it breaks the following scenario:

  • library in watch mode ng build my-library --watch
  • application with linked my-library using npm link
  • application running with ng serve

Starting from version 17.1 ng serve no longer detects file changes in my-library as it did before (webpack build). Looks like tsconfig paths mapping does not help either.
How to get Incremental builds functionality?

Edit:
@alan-agius4 17.1.2 fixes it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants