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.

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

Update tooltip.js #19541

Merged
merged 2 commits into from
Dec 22, 2016
Merged

Update tooltip.js #19541

merged 2 commits into from
Dec 22, 2016

Conversation

tracker1
Copy link
Contributor

Don't reference Tether via attachment to window, with the update one can import bootstrap providing the dependencies in webpack with:

new webpack.ProvidePlugin({
  $: 'jquery',
  jQuery: 'jquery',
  Tether: 'tether',
});

Then inside one's own bootstrap/globals, import 'bootstrap'; will simply work, and $/jQuery can be used from there.

I had wanted to do this, but also expose jQuery, Tether, etc when in development build in my code, but if I provide window.Tether, I can't then expose it to the outside...

Don't reference `Tether` via attachment to `window`, with the update one can import bootstrap providing the dependencies in webpack with:

```
new webpack.ProvidePlugin({
  $: 'jquery',
  jQuery: 'jquery',
  Tether: 'tether',
});
```

Then inside one's own bootstrap/globals, `import 'bootstrap';` will simply work, and $/jQuery can be used from there.

I had wanted to do this, but also expose jQuery, Tether, etc when in development build in my code, but if I provide `window.Tether`, I can't then expose it to the outside...
match project's style check
@vsn4ik
Copy link
Contributor

vsn4ik commented Mar 15, 2016

@@ -16,7 +16,7 @@ const Tooltip = (($) => {
* Check for Tether dependency
* Tether - http://github.hubspot.com/tether/
*/
if (window.Tether === undefined) {
if (typeof Tether === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just : if (Tether === undefined) ?
Your JSPerf prove it's faster than using typeof

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Johann-S That will throw an exception if Tether isn't defined...

if (someUndefinedValue === undefined) console.log('success')
// Uncaught ReferenceError someUndefinedValue is not defined

//this would work, but sloppier than the typeof
try {
  if (Tether){}
} catch(err) {
  throw new Error('Bootstrap tooltips require Tether (http://github.hubspot.com/tether/');
}

@tracker1
Copy link
Contributor Author

@vsn4ik in your example undef is declared, its value is initially set to undefined so that the test passes... if you try to run a truly undefined/undeclared value (no var declaration)...

if (iHaveNotBeenReferencedOrDefinedAnywhere == undefined) console.log('you win');

You will see...

Uncaught ReferenceError iHaveNotBeenReferencedOrDefinedAnywhere is not defined

Since Tether is supposed to be declared in another library, and will be undeclared if it isn't included, then doing if (Tether === undefined) ... will throw an uncaught exception.

Not to mention that the test in this case is really contrived, as it is only run a single time when the file in question is loaded, not millions of iterations.

@mdo
Copy link
Member

mdo commented Dec 21, 2016

@bardiharborow @Johann-S Do we need this? I'm unclear if this helps us at all.

@bardiharborow
Copy link
Member

bardiharborow commented Dec 21, 2016

I can't see any reason why we shouldn't merge this and close #20745. The issue here is that we want our code to work when Tether is loaded at both window.Tether (global, normal <script> inclusion) and Tether (local, webpack and various other bundlers). One of the many quirks with JavaScript is that member accesses (window.Tether) return undefined but local variable accesses throw an error. We can use typeof to work around that. I don't know why I didn't think of this as soon as I looked at #20745.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants