twbs
Fix this reference for functions
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
- [x] 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
- [x] I have read the contributing guidelines
- [x] My code follows the code style of the project (using
npm run lint) - [x] My change introduces changes to the documentation
- [x] I have updated the documentation accordingly
- [x] I have added tests to cover my changes
- [x] All new and existing tests passed
Related issues
Fixes #38720.
The failing test appears unrelated to my changes and passes locally for me - is this a flaky test? I'd appreciate it if a maintainer could rerun the workflow.
@mdo @GeoSot checking in to see if there's anything else I can do to help get this merged!
@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.
cc also @julien-deramond and @XhmikosR, looks like you two are the most active maintainers at the moment.
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 🙏
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.
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.
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.
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.