fix(@angular-devkit/schematics): running external schematics with yarn pnp #26109
Conversation
…n pnp This change addresses an issue encountered when running external schematics from a yarn pnp workspace. The function used to resolve a collection json using node used recursion in a way that it effectively walked itself into an exception. Then if the exception is the type it expected, it would keep going. This was flawed in that yarn with pnp throws a different type of error when it failed to load the mis-constructed collection path (e.g. `/node_modules/@schematics/angular/collection.json/package.json`). `ENOTDIR` instead of `MODULE_NOT_FOUND`. This process of intentionally / knowingly walking into an exception seems problematic in general. So, I addressed it by removing the recursion that was used mainly because there's a similar need to construct the collection path from a relative path in the package.json as there is to construct the collection path from a relative path that's passed in. Rather than leaning on the recursion to do this, I added the logic at the time we pull the schematics path from the package, and move on. Since the recursion was removed, the infinite recursion safety check at the start wasn't needed anymore. I've tested this in both yarn pnp and non-pnp environments.
alan-agius4
added
target: patch
|
Thank you for the contribution. |
|
@clydin thanks for the feedback. Do you happen to have an example of a package that has their collection declared in that way? Just so I understand the use case correctly. Or just a snippet of what that |
…n pnp This change addresses an issue encountered when running external schematics from a yarn pnp workspace. The function used to resolve a collection json using node used recursion in a way that it effectively walked itself into an exception. Then, if the exception is the type it expected, it would keep going. This was flawed in that yarn with pnp throws a different type of error when it failed to load the mis-constructed collection path (e.g. `/node_modules/@schematics/angular/collection.json/package.json`). `ENOTDIR` instead of `MODULE_NOT_FOUND`. This process of intentionally / knowingly walking into an exception seems problematic in general. So, I addressed it by first checking if the `schematics` entry in the package is a relative path. If it is, then don't construct the collection path from that. If entry is not relative, then assume it's pointing at another package and we need to recurse to get to the actual collection path. I've tested this in both yarn pnp and non-pnp environments.
|
I went ahead and added the recursion back in, but after the check for relative path. Let me know if you feel this addresses the package use-case. |
clydin
added
action: merge
|
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. |
This change addresses an issue encountered when running external schematics from a yarn pnp workspace. The function used to resolve a collection json using node used recursion in a way that it effectively walked itself into an exception. Then if the exception is the type it expected, it would keep going. This was flawed in that yarn with pnp throws a different type of error when it failed to load the mis-constructed collection path (e.g.
/node_modules/@schematics/angular/collection.json/package.json).ENOTDIRinstead ofMODULE_NOT_FOUND.This process of intentionally / knowingly walking into an exception seems problematic in
general. So, I addressed it by first checking if the
schematicsentry in the packageis a relative path. If it is, then don't construct the collection path from that.
If entry is not relative, then assume it's pointing at another package and we need
to recurse to get to the actual collection path.
I've tested this in both yarn pnp and non-pnp environments.
PR Checklist
Please check to confirm your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #25648
What is the new behavior?
Does this PR introduce a breaking change?
Fixes: #25648