fix(@ngtools/webpack): invalidate ngcc processor cache #16424
Conversation
filipesilva
added
the
target: patch
|
@clydin highlighted that a risk in these cache-related cases is to end up having stale or incorrect modules being used, but that letting this fix sit in RC for a while will help and we can always pull it. |
| @@ -54,15 +54,16 @@ export class NgccProcessor { | |||
| resolvedModule: ts.ResolvedModule | ts.ResolvedTypeReferenceDirective, | |||
| ): void { | |||
| const resolvedFileName = resolvedModule.resolvedFileName; | |||
| if (!resolvedFileName || moduleName.startsWith('.') || this._processedModules.has(moduleName)) { | |||
| if (!resolvedFileName || moduleName.startsWith('.') | |||
There was a problem hiding this comment.
This will cause a module to be processed multiple times which might slow down the compilation a bit, unless the typings are not flattened, example RxJs.
This is because this method will be called for every dts file resolved.
There was a problem hiding this comment.
Will ngcc actually process the module or skip it because it can see it's processed already? I think it skips it because the logs I saw only showed the ngcc logging messages once.
We could probably make cache take into account the relation between moduleName and resolvedFileName but I think it would introduce some extra risk actually. That's because the authority isn't this cache, but the ngcc cache. And it can actually happen that the whole module is already compiled but our cache doesn't know, since ngcc will compile a module dependencies anyway.
If we could instead invalidate whole modules directly, instead of filenames, I don't think there would be much risk. We can do that using ts.resolveModuleName, which is what the compiler host resolveModuleNames method that calls processModule does anyway. But this now means module resolution is invoked for file invalidations.
I'm not super keen on trying to make this cache much smarter in general because of the risks @clydin pointed out. The perf improvement seems a tradeoff too, unless we made it smarter. @alan-agius4 do you think it's worth it?
There was a problem hiding this comment.
@filipesilva it does skip it, however it will try to read the module and check that it is not already processed multiple times for each module causing quite a number of addition IO ops.
That said, I think we can leave it for now, and tackle it afterwards if we see a big slow down with a lot of dependencies.
There was a problem hiding this comment.
I think it is also important to note that resolveModuleName resolves to .d.ts files. Are all the relevant .d.t.s files watched in this situation? Libraries that don't fully adhere to the Angular package format for instance.
There was a problem hiding this comment.
I saw that .d.ts files for direct dependencies were being invalidated, but did not verify those other cases.
@petebacondarwin are there cases where the .d.ts resolved by ts.resolveModuleName is not the .d.ts file processed by ngcc?
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fix #16397