-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix transclude inefficiency #7647
Conversation
The transclusion target was sometimes being parsed twice when transcluding as text/plain Associated test results are also made more consistent
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
core/modules/widgets/transclude.js
Outdated
// "text/plain" is the plain text result of wikifying the text | ||
target = this.parseTransclusionTarget(); | ||
this.sourceText = target.text; | ||
this.parserType = target.type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 lines above seem to be the same code for all 3 states. They may be moved above the switch statement. Or is it intended to be that way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this.parseAsInline
should also initialized and moved outside the cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 lines above seem to be the same code for all 3 states. They may be moved above the switch statement. Or is it intended to be that way?
Yes that can be made a bit more efficient.
I think
this.parseAsInline
should also initialized and moved outside the cases.
I don't think so. It is not required on all code paths
target = this.parseTransclusionTarget(parseAsInline); | ||
this.sourceText = target.text; | ||
this.parserType = target.type; | ||
this.parseAsInline = target.parseAsInline; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this.parseAsInline
is only initialized if this case has been executed. Is that intentional? IMO undefined means "we do not know". The variable parseAsInline
is defined as true or false. Just a gut feeling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code relies on this.parseAsInline defaulting to false
Fixes #7592