refactor(core): guards stringify calls with ngDevMode #66958
Conversation
The `stringify` function is only needed for debugging purposes and should not be called in production mode.
| } | ||
|
|
||
| return new R3Injector(providers, parent || getNullInjector(), name || null, scopes); | ||
| return new R3Injector(providers, parent || getNullInjector(), source || null, scopes); |
There was a problem hiding this comment.
This value, as I understand it, is only used in dev
angular/packages/core/src/di/r3_injector.ts
Lines 401 to 410 in 63d828a
| try { | ||
| if (record.value === CIRCULAR) { | ||
| throw cyclicDependencyError(stringify(token)); | ||
| throw cyclicDependencyError(ngDevMode ? stringify(token) : ''); |
There was a problem hiding this comment.
We have a similar case here; the value is only used for dev mode.
angular/packages/core/src/render3/errors_di.ts
Lines 24 to 27 in 63d828a
packages/core/src/di/forward_ref.ts
Outdated
| (<any>forwardRefFn).__forward_ref__ = forwardRef; | ||
| (<any>forwardRefFn).toString = function () { | ||
| return stringify(this()); | ||
| return ngDevMode ? stringify(this()) : ''; |
There was a problem hiding this comment.
I’d only have a question in the case of toString. From what I reviewed, I couldn’t find any usage other than a possible development-time debug scenario.
Would this be necessary, similar to R3Injector, if we want to remove the stringify function from the final bundle
There was a problem hiding this comment.
I'm not a fan of return an empty string here.
If we were to drop that custom implem, I would rather wrap it entirely with a if(ngDevMode) { ... }.
There was a problem hiding this comment.
Updated both cases. Also, in R3Injector, I adding // @ts-ignore to suppress specific TypeScript warning
| return `R3Injector[${tokens.join(', ')}]`; | ||
| } | ||
| return `R3Injector[${tokens.join(', ')}]`; | ||
| } |
There was a problem hiding this comment.
We still have an issue here as toString will return undefined.
I think its's reasonable to return something like R3Injector[...]
There was a problem hiding this comment.
I think in this case should be enough return 'R3Injector[...] wdyt ?
There was a problem hiding this comment.
Yeah, that's what I had in mind.
3 participants
The
stringifyfunction is only needed for debugging purposes and should not be called in production mode.