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
fix (#1484 fixing 't.fn.datepicker is undefined' when loading bootstr… #2478
base: master
Are you sure you want to change the base?
fix (#1484 fixing 't.fn.datepicker is undefined' when loading bootstr… #2478
Conversation
…ing bootstrap-datepicker within requireJS even when $ has been declared as depedency)
@jdnierth I think your changes are not following the UMD pattern. |
dist/js/bootstrap-datepicker.js
Outdated
@@ -6,11 +6,11 @@ | |||
|
|||
(function(factory){ | |||
if (typeof define === 'function' && define.amd) { | |||
define(['jquery'], factory); | |||
define(['jquery'], factory(jQuery || window.jQuery || require('jquery'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ocarreterom , thanks for your comment but the UMD pattern is still in tact, the only change is to pass through variable declarations that can be different per environment. Happy to discuss if you have further feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ocarreterom I´m using this plugin in an AMD environment and it fails with the error message described in the bugs description without this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this line?
The second argument of define([dependecies], callback)
should be a function (a callback like factory
) to get the dependencies as params, not the result of the function factory()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ocarreterom, I did within a custom application but I´ll decline this PR until I´ve made sure I rule out any other reason why I experienced this error. Thanks for your patience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ocarreterom sorry I can`t decline this PR nor can I give it a label. In case you have more access permissions could you please put a label "on hold" on it? Many thanks in advance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @vsn4ik
dist/js/bootstrap-datepicker.js
Outdated
} else if (typeof exports === 'object') { | ||
factory(require('jquery')); | ||
factory(jQuery || window.jQuery || require('jquery')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jdnierth
Does this line not throw an error in a Node/Commonjs environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
facepalm you are correct it should throw an error in Node.js environment since there is no window
object, hence reverted this line. Thanks for the nudge @ocarreterom !
dist/js/bootstrap-datepicker.js
Outdated
} else { | ||
factory(jQuery); | ||
factory(jQuery || window.jQuery); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted this line to former factory(jQuery)
|
Description
Console.log screenshot
Project specific setup
HTML structure
RequireJS config excerpt
Custom module usage