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

changed scope of d4 var to work with how browserify compiles #45

Merged
merged 4 commits into from
Mar 29, 2016

Conversation

lomaxap
Copy link

@lomaxap lomaxap commented Mar 28, 2016

No description provided.

@heavysixer
Copy link
Owner

Did you manually change the references to d4 in the compiled file? Because if so that won't work with the way the lib is compiled from parts.

@lomaxap
Copy link
Author

lomaxap commented Mar 28, 2016

no, i only changed the base.js file and compiled with grunt

@heavysixer
Copy link
Owner

Ok the compiled file looked a bit odd. Let me test your branch locally but if everything works i'll make a new release. Thanks!

@heavysixer
Copy link
Owner

One final question, is there a way to give a minimal browserify test for this? How did you confirm this worked?

@lomaxap
Copy link
Author

lomaxap commented Mar 29, 2016

good question. I found this tool browserify-test and ran this test which passed:

var expect = require( 'chai' ).expect;

describe('browserify test', function() {
  it('browserify should compile d4', function() {
       var d4 = require( '../lib/d4.js' );
      expect(d4).to.not.be.an('undefined');
  });
});

let me know if that works.

@heavysixer
Copy link
Owner

Awesome, can you integrate that with the existing test suite and append the PR you sent me?

Thanks again for your hard work!

Best Regards,
Mark

On Mar 29, 2016, at 10:47 AM, Andrew Lomax [email protected] wrote:

good question. I found this tool browserify-test https://github.com/alekseykulikov/browserify-test and ran this test which passed:

var expect = require( 'chai' ).expect;

describe('browserify test', function() {
it('browserify should compile d4', function() {
var d4 = require( '../lib/d4.js' );
expect(d4).to.not.be.an('undefined');
});
});
let me know if that works.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub #45 (comment)

@lomaxap
Copy link
Author

lomaxap commented Mar 29, 2016

ok. no problem. I need to add grunt-browserify and a new task to run during grunt test. is that fine with you?

@heavysixer
Copy link
Owner

Perfect, I’ll confirm it works for me and then merge.

On Mar 29, 2016, at 12:04 PM, Andrew Lomax [email protected] wrote:

ok. no problem. I need to add grunt-browserify and a new task to run during grunt test. is that fine with you?


You are receiving this because you commented.
Reply to this email directly or view it on GitHub #45 (comment)

@heavysixer heavysixer merged commit 5741483 into heavysixer:master Mar 29, 2016
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