Conversation
7183953 to
8497f22
Compare
…teUrlTree with invalid parameters In angular#38227 the signatures of `navigateByUrl` and `createUrlTree` were updated to exclude unsupported properties from their `extras` parameter. This migration looks for the relevant method calls that pass in an `extras` parameter and drops the unsupported properties. **Before:** ``` this._router.navigateByurl(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fpull%2F%27%2F%27%2C%2520%257BskipLocationChange%3A%2520false%2C%2520fragment%3A%2520%27foo%27%257D); ``` **After:** ``` this._router.navigateByUrl('/', { /* Removed unsupported properties by Angular migration: fragment. */ skipLocationChange: false }); ``` These changes also move the method call detection logic out of the `Renderer2` migration and into a common place so that it can be reused in other migrations.
8497f22 to
e65956c
Compare
atscott
left a comment
There was a problem hiding this comment.
Thanks for this! Just one comment (and a documentation request) but otherwise LGTM.
| runTSLint(true); | ||
|
|
||
| const content = getFile('/index.ts'); | ||
| expect(content).toContain( |
There was a problem hiding this comment.
I think the risk of a NavigationExtras variable being re-used is just about zero, but do you know what would happen if a NavigationExtras variable were used by both navigateByUrl and createUrlTree?
Actually, this is kind of the case of Router#navigate:
return this.navigateByurl(/sitelet?url=https%3A%2F%2Fgithub.com%2Fangular%2Fangular%2Fpull%2Fthis.createUrlTree%28commands%2C%2520extras), extras);
Could you add a test or two for this case?
I wonder if it would be better to have a type cast for ts.Identifiers at the call location instead of migrating the literal variable declarations.
There was a problem hiding this comment.
As things are set up right now, it'll try to migrate both the navigateByUrl and createUrlTree calls so it can end up removing too much. I can make it either be a noop if it's used in both places, or add a TODO comment for people to migrate manually, or potentially give one method precedence over the other.
I suppose that casting it would work too, but I decided not to follow that route, because it just solves the compilation error, it doesn't actually correct the user's problem.
There was a problem hiding this comment.
I think it's extremely rare/unlikely that this might happen so I'll leave it up to you to decide if you want to handle it. If you do, I think a no-op would be the approach to go with. At worst, the application wouldn't compile but you'd at least be pointed directly to where a fix is needed. Removing properties might cause unexpected/silent breakages unless they're caught in unit tests.
devversion
left a comment
There was a problem hiding this comment.
LGTM. Just a few minor comments
| if ((ts.isPropertyAssignment(property) || ts.isShorthandPropertyAssignment(property)) && | ||
| (ts.isStringLiteral(property.name) || ts.isNumericLiteral(property.name) || | ||
| ts.isIdentifier(property.name))) { | ||
| if (!property.name || allowedProperties.has(property.name.text)) { |
There was a problem hiding this comment.
Why is !property.name needed here? From past migrations I know that ts.is<...> functions throw if they receive undefined, so we might want to move that above.
There was a problem hiding this comment.
I think it's a leftover that I hadn't cleaned up after I reworked the code. Removed.
crisbeto
left a comment
There was a problem hiding this comment.
I've reworked it based on the feedback.
| if ((ts.isPropertyAssignment(property) || ts.isShorthandPropertyAssignment(property)) && | ||
| (ts.isStringLiteral(property.name) || ts.isNumericLiteral(property.name) || | ||
| ts.isIdentifier(property.name))) { | ||
| if (!property.name || allowedProperties.has(property.name.text)) { |
There was a problem hiding this comment.
I think it's a leftover that I hadn't cleaned up after I reworked the code. Removed.
| runTSLint(true); | ||
|
|
||
| const content = getFile('/index.ts'); | ||
| expect(content).toContain( |
There was a problem hiding this comment.
As things are set up right now, it'll try to migrate both the navigateByUrl and createUrlTree calls so it can end up removing too much. I can make it either be a noop if it's used in both places, or add a TODO comment for people to migrate manually, or potentially give one method precedence over the other.
I suppose that casting it would work too, but I decided not to follow that route, because it just solves the compilation error, it doesn't actually correct the user's problem.
…nd createUrlTree with invalid parameters
46cb893 to
4d92fa1
Compare
…nd createUrlTree with invalid parameters
|
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 #38227 the signatures of
navigateByUrlandcreateUrlTreewere updated to exclude unsupported properties from theirextrasparameter. This migration looks for the relevant method calls that pass in anextrasparameter and drops the unsupported properties.Before:
After:
These changes also move the method call detection logic out of the
Renderer2migration and into a common place so that it can be reused in other migrations.