Fix this reference for functions

[nwalters512]

Description

This PR fixes the bug described in #38720. Functions like content and title weren't being called with the correct this reference.

Motivation & Context

As far as I can tell, #36652 introduced this regression. It switched from calling the function with a specific this value to calling it with that value as the first argument.

While this appears to have been unintentional, folks may have started relying on that first argument as a workaround, and thus it could be considered part of the API. I didn't do anything about that yet, but it might be reasonable to retain both the old and new behavior (that is, for functions like content, call it with both a specific this value and an explicit argument). I can make that change and update the documentation if requested.

I repurposed the existing execute() function to support this. If a second argument is provided, it will be an array, and the first value will be treated as the this argument. This is done by just passing the entire arguments array to call. This required adding undefined as the first value in some places that don't expect an specific this value.

Type of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Refactoring (non-breaking change)
• Breaking change (fix or feature that would change existing functionality)

Checklist

• I have read the contributing guidelines
• My code follows the code style of the project (using npm run lint)
• My change introduces changes to the documentation
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed

Related issues

Fixes #38720.

[XhmikosR]

/CC @GeoSot

[GeoSot]

Suggested change

	return execute(arg, [undefined, this])
	return typeof arg === 'function' ? arg.call(this._element) : arg

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

[nwalters512]

This callsite doesn't actually matter, AFAICT. There are two calls where the this argument is important:

• _resolvePossibleFunction in js/src/tooltip.js
• The call to execute(this._config.placement, ...) in _createPopper in js/src/tooltip.js

I can update those two call sites to use the typeof arg === 'function' ... pattern if you'd prefer that I avoid changing execute?

[nwalters512]

@mdo @GeoSot 👋 anything I can do to help this along?

[mdo]

Seems good to go @GeoSot?

[nwalters512]

@mdo @GeoSot checking in to see if there's anything else I can do to help get this merged!

[Precidata]

Happy 1 year anniversary for this bug / PR :)

[nwalters512]

@mdo @GeoSot checking in once more! I'd really like to see this land. Let me know if there's anything at all I can do to help this along.

[nwalters512]

cc also @julien-deramond and @XhmikosR, looks like you two are the most active maintainers at the moment.

[julien-deramond]

Sorry for the very long delay; I know it's frustrating. This PR is on my to-do list along with many other issues and PRs for v5.3.x. Unfortunately, my spare time dedicated to Bootstrap isn't as extensive as I'd like it to be, which is frustrating for me too. Thanks for your patience and your contribution 🙏

[XhmikosR]

This needs @GeoSot approval since it was his patch that introduced the breakage. Last time I talked to @GeoSot, he didn't consider this an issue or something like that.

I hate breaking changes myself, but at this point, I'm not sure what the rest of us can do, especially without a test case.

EDIT: I see there's a test, I forgot there was one, sorry.

[nwalters512]

A breaking change in a minor release is painful, but would have been much less so if the change were documented in the release notes and the documentation for popovers/tooltips/etc. were updated. As it stands, I killed about a day last year trying to figure out why Bootstrap 5.3 broke all my popovers. One way or another, I'd like to save others from that pain.

If you're unwilling to accept this patch, let me know and I'll open a PR with a) tests for the new, current behavior (no this reference, instances passed as first argument), b) retroactively updated release notes for 5.3, and c) updated documentation.

[adriweb]

If not in a minor release, when would the fix be landing?
Because this also broke prod sites for me, and I've thus sadly had to stick to an older version.

[julien-deramond]

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 Util.execute function only, I'm not that comfortable with an extra first parameter that seems useless. Considering this function in isolation, we would probably expect it to work like this:

• Util.execute(<function reference>) or Util.execute(<function reference>, []) when there's no args
• Util.execute(<function reference>, [<arg 1>]
• Util.execute(<function reference>, [<arg 1>, <arg 2>]
• etc.

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 undefined first argument in the list should be removed?

[nwalters512]

I think my rationale for doing it this way is that the args array directly mirrors how Function.prototype.call works, where the first argument is this and the remaining ones are the arguments.

I'm entirely happy to change the interface, e.g. by updating Util.execute to explicitly take a value to be used at this. For instance, we could update Util.execute to look like this:

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 Function.prototype.call) would make the most sense.

It's also possible that Util.execute is simply the wrong thing to be using in the callbacks for tooltips. As mentioned in #38725 (comment), I'm happy to switch those callsites to essentially inline it instead:

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

[julien-deramond]

It wasn't immediately clear to me, but now I see the value in mirroring Function.prototype.call as you suggested.

const resultFoo = Util.execute(functionFoo, undefined, [4, 5]);

With your explanation, I'd say that your version in this PR, Util.execute(functionFoo, [undefined, 4, 5]); is a better fit because it aligns more closely with Function.prototype.call.

[...] but I think putting it before the arguments (again to mirror Function.prototype.call) would make the most sense.

Yes it would.

It's also possible that Util.execute is simply the wrong thing to be using in the callbacks for tooltips. As mentioned in #38725 (comment), I'm happy to switch those callsites to essentially inline it instead

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 typeof arg === 'function' checks. Thus, retaining execute seems reasonable if we want to maintain that approach.

You've already done the work to keep execute, so given that I don't have a strong preference at the moment, I'll continue to review the use cases from this angle.

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.

[nwalters512]

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 this. If there's a desire to keep the accidentally-introduced new behavior, I'll add tests and update docs to reflect that.

My 2 cents: let's support the new behavior as well.

• Folks might have started relying on that as a workaround.
• Passing explicit arguments feels more like "modern" JS, whereas setting this feels like it might have been born from the jQuery era.

[julien-deramond]

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):

• Someone migrating from v5.1 to this new v5.3.x could use everything as described in the documentation
• Someone who had to use a workaround could install the new v5.3.x transparently

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.

[nwalters512]

Maybe a reasonable middle ground is to implement+test both but avoid documenting the "new" behavior until either a future minor/major release?

[julien-deramond]

Yeah, that was the idea I had in mind.

1. Implement + test both so that it works for all use cases (migrating from versions before the breaking change, but also supporting transparently workarounds that have been applied). It would give us some space if there are mistakes with the new implementation (as long as the old implementation is fixed and the previous doc still works).

2. Once we have ensured stability, we can document and present the "new" behavior as a minor feature in v5.4.

If this approach sounds reasonable and feasible to you and others involved in this PR, I suggest we proceed with this strategy in mind.

[nwalters512]

I updated the implementation and tests in a1a57a0.

[julien-deramond]

Thanks a lot again @nwalters512 for this PR and your patience!

Based on our discussions and my review, it looks good to me. I've created this CodePen to demonstrate that the original issue is resolved. Additionally, I haven't found any regressions during my manual testing, although it's challenging to cover all use cases.

In my opinion, the safest approach to move forward would be to include this in v5.3.4 (along with #40623) and continue our work targeting v5.3.5. If any issues are identified with this fix in the meantime, we can address them in a subsequent v5.3.x release.

I let the @twbs/js-review team finalize this review and/or merge.

[julien-deramond]

This topic has been discussed outside of GitHub. In summary, we have mixed feelings because this was an undocumented functionality that some users rely on due to the permissive nature of the interface, leading to an unintended regression when we refactored some aspects. On the other hand, within our JavaScript philosophy, providing internal access to a component is not considered good practice.

Given these factors, the most favorable course of action is to merge this PR for version 5, as it will address the regression for those relying on it. However, we may reconsider or modify this approach in version 6. This could involve changing the JavaScript interface or better documenting these types of usages.