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

Added support to AMD and CommonJS #356

Merged
merged 1 commit into from
Oct 14, 2016

Conversation

McGiogen
Copy link
Contributor

@McGiogen McGiogen commented Oct 4, 2016

I'm using webpack and without this I'm not able to do

var table = $('#dtTable').DataTable(...);
yadcf.init(table , [{column_number : 0}]);

because yadcf is not available.

I've followed this guide.

Ps: I've not tested this in a simple browser environment but it should works.

Update: there is a problem, inline codes like onclick="yadcf.stopPropagation(event);" can't works without the global yadcf variable.

@vedmack
Copy link
Owner

vedmack commented Oct 4, 2016

So you can do something like this (solve global yadcf)

var yadcf;
(function (root, factory) {
.
.
.
}(this, function($) {

yadcf = (function ($) {
.
.
.
}(jQuery));
return yadcf;
}));

@McGiogen
Copy link
Contributor Author

McGiogen commented Oct 4, 2016

I've fixed the global with the following lines because Webpack (and I think both AMD and CommonJS) automatically wraps the code in a closure, so simple variables became automatically locals instead of globals.

if (window) {
    window.yadcf = yadcf;
}

Now it should be ready to be merged.

@vedmack vedmack merged commit 2b82c06 into vedmack:master Oct 14, 2016
@vedmack
Copy link
Owner

vedmack commented Oct 14, 2016

Thanks!

@McGiogen
Copy link
Contributor Author

When do you think we will see this code published (on npm)?

@vedmack
Copy link
Owner

vedmack commented Oct 25, 2016

It will be in a few days

@vedmack
Copy link
Owner

vedmack commented Nov 3, 2016

0.9.1 is published on npm

@McGiogen McGiogen deleted the support-amd-commonjs branch November 3, 2016 09:38
@McGiogen
Copy link
Contributor Author

McGiogen commented Dec 5, 2016

Hi vedmark, I'm still here.
I found that momentjs is not usable with amd and commonjs because I've not insert it as a dependency. I'm doing another pull request to resolve the problem but in that way momentjs will became a must-have dependency (not optional) if yadcf is used with amd, commonjs or webpack (my case) because I've not found a way to do it for all those frameworks and pheraps this way doesn't exist.
There will be no changes if it is used in the classic mode.

@vedmack
Copy link
Owner

vedmack commented Dec 5, 2016

Hi,
Please do some additional googling / maybe SO to see how its possible not to make moment a "must-have " dependency , because it doesn't sound good

@McGiogen
Copy link
Contributor Author

McGiogen commented Dec 5, 2016

I created a new pull request, it should work as expected.

Ps: Why there are two "jquery.dataTables.yadcf.js" files? One is under "src/" folder...

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

Successfully merging this pull request may close these issues.

2 participants