-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[@kbn/handlebars] Ensure only decorators have an options.args
property
#147791
Conversation
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
To update your PR or re-run it, just comment with: cc @watson |
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.
LGTM!
} | ||
} | ||
|
||
// @ts-expect-error: Property 'lookupProperty' does not exist on type 'HelperOptions' | ||
delete options.lookupProperty; // There's really no tests/documentation on this, but to match the upstream codebase we'll remove `lookupProperty` from the decorator context |
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.
Does this change anything / Would it break if you don't follow original Handlebars?
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.
No it doesn't change anything. I think it's just an implementation detail/difference between our implementation and the upstream implementation. But I thought it doesn't hurt to try and match the upstream API. In upstream, lookupProperty
is added to options
in a separate step for helpers, whereas we just add it up front when creating the options
object.
…rty (elastic#147791) After adding support for decorators an `args` property has added to `HelperOptions` (which is shared with decorators). To not leak this into regular helpers and to align with the upstream handlebars, this PR removes the `args` property from regular helpers so that it's only visible to decorators. (cherry picked from commit 800f451)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
… property (#147791) (#147799) # Backport This will backport the following commits from `main` to `8.6`: - [[@kbn/handlebars] Ensure only decorators have an `options.args` property (#147791)](#147791) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Thomas Watson","email":"watson@elastic.co"},"sourceCommit":{"committedDate":"2022-12-19T19:40:18Z","message":"[@kbn/handlebars] Ensure only decorators have an `options.args` property (#147791)\n\nAfter adding support for decorators an `args` property has added to\r\n`HelperOptions` (which is shared with decorators). To not leak this into\r\nregular helpers and to align with the upstream handlebars, this PR\r\nremoves the `args` property from regular helpers so that it's only\r\nvisible to decorators.","sha":"800f45180cb1706a67f264ba5cee95c1a2031b8a","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:prev-minor","v8.7.0"],"number":147791,"url":"#147791 Ensure only decorators have an `options.args` property (#147791)\n\nAfter adding support for decorators an `args` property has added to\r\n`HelperOptions` (which is shared with decorators). To not leak this into\r\nregular helpers and to align with the upstream handlebars, this PR\r\nremoves the `args` property from regular helpers so that it's only\r\nvisible to decorators.","sha":"800f45180cb1706a67f264ba5cee95c1a2031b8a"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"#147791 Ensure only decorators have an `options.args` property (#147791)\n\nAfter adding support for decorators an `args` property has added to\r\n`HelperOptions` (which is shared with decorators). To not leak this into\r\nregular helpers and to align with the upstream handlebars, this PR\r\nremoves the `args` property from regular helpers so that it's only\r\nvisible to decorators.","sha":"800f45180cb1706a67f264ba5cee95c1a2031b8a"}}]}] BACKPORT--> Co-authored-by: Thomas Watson <watson@elastic.co>
…rty (elastic#147791) After adding support for decorators an `args` property has added to `HelperOptions` (which is shared with decorators). To not leak this into regular helpers and to align with the upstream handlebars, this PR removes the `args` property from regular helpers so that it's only visible to decorators.
After adding support for decorators an
args
property has added toHelperOptions
(which is shared with decorators). To not leak this into regular helpers and to align with the upstream handlebars, this PR removes theargs
property from regular helpers so that it's only visible to decorators.