fix(cdk/schematics): errors when attempting to read some files #19783
Conversation
crisbeto
added
P2
googlebot
added
the
cla: yes
In some cases the schematics fail to read a file which results in an error, because we weren't guarding against it properly. We had the correct types in place, but the tsconfig didn't have `strictNullChecks` enabled which prevented the compiler from reporting the errors. Fixes angular#19779.
crisbeto
force-pushed
the
19779/cdk-schematics-null-pointer-errors
branch
from
5dfa833 to
8b98f30
Compare
| // In case the stylesheet does not exist in the file system, skip it gracefully. | ||
| if (!this._fileSystem.exists(stylesheetPath)) { | ||
| return; | ||
| if (stylesheet) { |
There was a problem hiding this comment.
The change in general is good (enabling strict), but I wonder how this issue surfaces? Do we understand how this can happen? We asserted that a file exists in the virtual tree, and then read from it? Reading the file from the tree should not be null, or is this an edge case when a file is not readable somehow?
There was a problem hiding this comment.
The assertion here was fine, but I think it failed on the one below where we didn't have an assertion.
There was a problem hiding this comment.
Unless I miss something, we had a check before we invoked resolveExternalStylesheet. And the other one just accepted a glob of source files which are guaranteed to exist in FS.
Maybe the glob returned source files which exist on FS but are not available in the virtual file system. That would seem most likely. It would be good understanding this as it could potentially be a bug in the devkit, or general reasoning required for future schematic work.
There was a problem hiding this comment.
I guess it depends on whether the resolve call here corresponds to a path.resolve from Node which won't check the file system, or whether it's something different that actually does the lookup. If it's the latter, your explanation sounds plausible. Either way, I think that we should get this in for the next release so people that are updating to v10 don't run into it.
There was a problem hiding this comment.
The resolve call should not be relevant here. The list of additionalStylesheetPaths is based on a real file system query, and resolve just turns the absolute FS path into a virtual FS workspace relative path.
So there are two options: Either the files are somehow not readable and are not added to the virtual FS tree, or our path resolution logic is flawed. And this is the point/issue I think we should figure out. Fixing the issue is great, but understanding the root cause seems beneficial. To be clear: From the beginning on I agreed that this is a good change to land either way, but it would be still worth figuring this out I assume. I'll ask the issue reporter for more information.
Also I think since this has been reported only once and seems very edgy/surprising, this is probably not P2 worthy.
devversion
added
P3
In some cases the schematics fail to read a file which results in an error, because we weren't guarding against it properly. We had the correct types in place, but the tsconfig didn't have `strictNullChecks` enabled which prevented the compiler from reporting the errors. Fixes #19779.
|
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. |
In some cases the schematics fail to read a file which results in an error, because we weren't guarding against it properly. We had the correct types in place, but the tsconfig didn't have
strictNullChecksenabled which prevented the compiler from reporting the errors.Fixes #19779.