Skip to content
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.

Sign up for GitHub

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 this reference for functions #38725

Merged
merged 17 commits into from
Jul 19, 2024
Merged

Fix this reference for functions #38725

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9148528
Fix this reference for functions
nwalters512 Jun 7, 2023
dcf21d6
Merge branch 'main' into fix-function-this-reference
nwalters512 Nov 28, 2023
f2411f7
Merge branch 'main' into fix-function-this-reference
nwalters512 Dec 27, 2023
52b3d48
Merge branch 'main' into fix-function-this-reference
nwalters512 Jan 21, 2024
0793b5d
Merge branch 'main' into fix-function-this-reference
XhmikosR Feb 20, 2024
8c85eb7
Merge branch 'main' into fix-function-this-reference
nwalters512 Feb 23, 2024
bf47ca9
Merge branch 'main' into fix-function-this-reference
nwalters512 Feb 28, 2024
407305c
Merge branch 'main' into fix-function-this-reference
nwalters512 Mar 21, 2024
fde01d9
Merge branch 'main' into fix-function-this-reference
nwalters512 Apr 15, 2024
878b596
Merge branch 'main' into fix-function-this-reference
nwalters512 May 1, 2024
3dc6732
Merge branch 'main' into fix-function-this-reference
nwalters512 Jun 11, 2024
d828f67
Merge branch 'main' into fix-function-this-reference
nwalters512 Jun 14, 2024
03075be
Improve unit tests for dropdown/tooltip popperConfig function
nwalters512 Jun 19, 2024
a1a57a0
Add/improve tests
nwalters512 Jun 20, 2024
051f5cd
Merge branch 'main' into fix-function-this-reference
nwalters512 Jun 25, 2024
2b2ca8a
Merge branch 'main' into fix-function-this-reference
julien-deramond Jul 12, 2024
66ed889
Merge branch 'main' into fix-function-this-reference
julien-deramond Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion js/src/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ class Dropdown extends BaseComponent {

return {
...defaultBsPopperConfig,
...execute(this._config.popperConfig, [defaultBsPopperConfig])
...execute(this._config.popperConfig, [undefined, defaultBsPopperConfig])
julien-deramond marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
2 changes: 1 addition & 1 deletion js/src/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ class Tooltip extends BaseComponent {

return {
...defaultBsPopperConfig,
...execute(this._config.popperConfig, [defaultBsPopperConfig])
...execute(this._config.popperConfig, [undefined, defaultBsPopperConfig])
}
}

Expand Down
2 changes: 1 addition & 1 deletion js/src/util/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ const defineJQueryPlugin = plugin => {
}

const execute = (possibleCallback, args = [], defaultValue = possibleCallback) => {
return typeof possibleCallback === 'function' ? possibleCallback(...args) : defaultValue
return typeof possibleCallback === 'function' ? possibleCallback.call(...args) : defaultValue
}

const executeAfterTransition = (callback, transitionElement, waitForTransition = true) => {
Expand Down
2 changes: 1 addition & 1 deletion js/src/util/template-factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ class TemplateFactory extends Config {
}

_resolvePossibleFunction(arg) {
return execute(arg, [this])
return execute(arg, [undefined, this])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nwalters512 nwalters512 Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The 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 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?

}

_putElementInTemplate(element, templateElement) {
Expand Down
27 changes: 27 additions & 0 deletions js/tests/unit/popover.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,33 @@ describe('Popover', () => {
})
})

it('should call content and title functions with correct this value', () => {
return new Promise(resolve => {
fixtureEl.innerHTML = '<a href="#" data-foo="bar">BS twitter</a>'

const popoverEl = fixtureEl.querySelector('a')
const popover = new Popover(popoverEl, {
title() {
return this.dataset.foo
},
content() {
return this.dataset.foo
}
})

popoverEl.addEventListener('shown.bs.popover', () => {
const popoverDisplayed = document.querySelector('.popover')

expect(popoverDisplayed).not.toBeNull()
expect(popoverDisplayed.querySelector('.popover-header').textContent).toEqual('bar')
expect(popoverDisplayed.querySelector('.popover-body').textContent).toEqual('bar')
resolve()
})

popover.show()
})
})

it('should show a popover with just content without having header', () => {
return new Promise(resolve => {
fixtureEl.innerHTML = '<a href="#">Nice link</a>'
Expand Down
13 changes: 13 additions & 0 deletions js/tests/unit/tooltip.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1337,6 +1337,19 @@ describe('Tooltip', () => {

expect(tooltip._getTitle()).toEqual('test')
})

it('should call title function with correct this value', () => {
fixtureEl.innerHTML = '<a href="#" rel="tooltip" data-foo="bar"></a>'

const tooltipEl = fixtureEl.querySelector('a')
const tooltip = new Tooltip(tooltipEl, {
title() {
return this.dataset.foo
}
})

expect(tooltip._getTitle()).toEqual('bar')
})
})

describe('getInstance', () => {
Expand Down
4 changes: 2 additions & 2 deletions js/tests/unit/util/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Member

@julien-deramond julien-deramond Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

@nwalters512 nwalters512 Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

Choose a reason for hiding this comment

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

  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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'
Expand Down
Loading