fix(@angular-devkit/schematics): throw `InvalidCollectionJsonExceptio… #11841
Conversation
| @@ -102,7 +102,11 @@ export class NodeModulesEngineHost extends FileSystemEngineHostBase { | |||
| return collectionPath; | |||
| } | |||
| } catch (e) { | |||
| if (!(e instanceof FileDoesNotExistException)) { | |||
| throw new InvalidCollectionJsonException(name, collectionPath); | |||
There was a problem hiding this comment.
Could you just rethrow the error instead? The sub error can be a JSON error which contains more information (line and character), or another error.
There was a problem hiding this comment.
I initially was gonna do did that, the only drawback was the in the error there won’t be information about which files cause the JSON error.
An alternative would be that InvalidCollectionJsonException would accept another parameter, and eventually if error.message is present it will be concatinated with original message that InvalidCollectionJsonException is showing to the user.
What do yo think?
There was a problem hiding this comment.
Something similar to this error #11861 with no context of which file is the problem.
There was a problem hiding this comment.
I think either solution would be better:
- An alternative would be that InvalidCollectionJsonException would accept another parameter of type
InvalidJsonExceptionand transfer over the info, or - A separate new
InvalidJsonFileExceptionwhich takes a path and the json exception.
I'm fine with either. I have a slight preference to 1.
There was a problem hiding this comment.
I also prefer for the first option. I have pushed an updated. When you said transfer over the info I was not sure if you were referring the error message or stack or both. For the time being I did the message.
|
@hansl, can you re-review this please? Thanks. |
|
Hi @alan-agius4! This PR has merge conflicts due to recent upstream merges. |
alan-agius4
added
the
target: patch
| @@ -91,16 +103,16 @@ export class SchematicNameCollisionException extends BaseException { | |||
| * all other EngineHost provided by the tooling part of the Schematics library. | |||
| */ | |||
| export abstract class FileSystemEngineHostBase implements | |||
| EngineHost<FileSystemCollectionDescription, FileSystemSchematicDescription> { | |||
| EngineHost<FileSystemCollectionDescription, FileSystemSchematicDescription> { | |||
There was a problem hiding this comment.
Why all the style changes? Your PR should only include code you changed.
There was a problem hiding this comment.
Yeah agreed, must have done auto format when fixing the rebase conflict.
Will fix this later
…n` when collection file is invalid Closes #11818
|
|
|
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. |
throw
InvalidCollectionJsonExceptionwhen collection file is invalidCloses #11818
Not quite sure, how can I test this, since I cannot use the
node-modules-test-engine-host