-
-
Notifications
You must be signed in to change notification settings - Fork 79.2k
Fix this reference for functions #38725
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
Changes from all commits
9148528
dcf21d6
f2411f7
52b3d48
0793b5d
8c85eb7
bf47ca9
407305c
fde01d9
878b596
3dc6732
d828f67
03075be
a1a57a0
051f5cd
2b2ca8a
66ed889
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -143,7 +143,7 @@ class TemplateFactory extends Config { | |||||
} | ||||||
|
||||||
_resolvePossibleFunction(arg) { | ||||||
return execute(arg, [this]) | ||||||
return execute(arg, [undefined, this]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I suppose it is better to change only this, as it is count as a regression, and add a test, instead of messing all the rest calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This callsite doesn't actually matter, AFAICT. There are two calls where the
I can update those two call sites to use the |
||||||
} | ||||||
|
||||||
_putElementInTemplate(element, templateElement) { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -521,10 +521,10 @@ describe('Util', () => { | |
|
||
it('should execute if arg is function & return the result', () => { | ||
const functionFoo = (num1, num2 = 10) => num1 + num2 | ||
const resultFoo = Util.execute(functionFoo, [4, 5]) | ||
const resultFoo = Util.execute(functionFoo, [undefined, 4, 5]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in the process of understanding what was there before and what's changing in this PR. It's a bit challenging to think about all the use cases, so I hope you won't mind if my remarks or questions are not yet precise or accurate. If we don't think about the entire context of this PR and focus on this
I'm not saying it doesn't have impacts or that it's not technically feasible, but in terms of pure interface design, don't you think this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my rationale for doing it this way is that the I'm entirely happy to change the interface, e.g. by updating const execute = (possibleCallback, thisArg = undefined, args = [], defaultValue = possibleCallback) Then, the test case you commented on here would look like this: const resultFoo = Util.execute(functionFoo, undefined, [4, 5]); Of course, we could reorder the arguments, but I think putting it before the arguments (again to mirror It's also possible that typeof arg === 'function' ? arg.call(this._element) : arg
// If we want to retain backwards compatibility with the new API accidentally
// introduced in 5.3, we can do this instead:
typeof arg === 'function' ? arg.call(this._element, this._element) : arg There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It wasn't immediately clear to me, but now I see the value in mirroring
With your explanation, I'd say that your version in this PR,
Yes it would.
I'm not the sole decision-maker here, but from my perspective as a non-JS expert, either approach could work. The initial idea with #36652 was to consolidate functionality and avoid repetitive You've already done the work to keep Note: I'm currently trying to reach the JS maintainers to discuss how we can proceed and finalize this PR with their input and yours. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds great! I'll be ready to chat with you or the other JS maintainers to find an approach that'll make everyone happy. Just so it isn't overlooked, a main decision to make here is whether or not Bootstrap will continue calling these functions with the tooltip/popover/etc. as an explicit argument as well as the old behavior of setting My 2 cents: let's support the new behavior as well.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, so we got a regression where folks had to use a workaround. The basic principle of a fix is that when there's a new patch release (v5.3.x), folks can then remove their workaround so that it works like before. Since we didn't release a patch promptly, this workaround might now be seen as a de facto part of the API. Even if it isn't officially recognized as such, we may decide not to inconvenience users who have relied on and adapted to this workaround for an extended period. Otherwise, they might perceive the new patch as a disruptive change/breaking change as well, if not retro-compatible. 🙃 Considering these factors, I'd say supporting both the old and the new behavior would ensure a seamless transition for everyone installing the patch version (and we might even consider later on deprecating the old behavior in a future major Bootstrap version, for example):
Yet, I'm wondering whether introducing the new behavior in the documentation should wait until v5.4.0. Even though the feature could be currently present but hidden, delaying its documentation could help us avoid unnecessary remarks and comments. Not entirely sure if this is the right strategy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a reasonable middle ground is to implement+test both but avoid documenting the "new" behavior until either a future minor/major release? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that was the idea I had in mind.
If this approach sounds reasonable and feasible to you and others involved in this PR, I suggest we proceed with this strategy in mind. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the implementation and tests in a1a57a0. |
||
expect(resultFoo).toBe(9) | ||
|
||
const resultFoo1 = Util.execute(functionFoo, [4]) | ||
const resultFoo1 = Util.execute(functionFoo, [undefined, 4]) | ||
expect(resultFoo1).toBe(14) | ||
|
||
const functionBar = () => 'foo' | ||
|
Uh oh!
There was an error while loading. Please reload this page.