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

use gulp to build #135

Merged
merged 2 commits into from May 29, 2015
Merged

use gulp to build #135

merged 2 commits into from May 29, 2015

Conversation

az7arul
Copy link
Collaborator

@az7arul az7arul commented Mar 22, 2015

@jprichardson I think this needs a good look.

  • moved to gulp for building it
  • uses browserify #134
  • changes main to dist/* from lib/*

You might want to make sure bower/component/npm works with this and the test/browser.test.html is ok.

@jprichardson
Copy link
Owner

I like this a lot! Before we merge, what do you think about removing bower (AMD) and component support? People could use Webpack/Browserify to add bower (AMD) support back. We could even create another repo called string-amd or string-bower for something like that. But build tools are sophisticated enough these days that libraries should be able to build using one module format and then provide interfaces for the rest through the build process. It would simplify our lives some. Thoughts?

@az7arul
Copy link
Collaborator Author

az7arul commented Mar 22, 2015

I think by default it should support both. We could work on something like lodash-cli to give user ability to get custom builds or use that to push code to repo's like string-amd. We should look at how the lodash folks are doing things before making a decision.

@jprichardson
Copy link
Owner

I agree that using lodash as an ideal model and I'm in favor of that. This PR looks good as far as I can tell.

@az7arul
Copy link
Collaborator Author

az7arul commented Mar 23, 2015

Can you take a look at the test/browser.test.html ? seems to be its broken
because of this not sure though.

On Tue, Mar 24, 2015 at 1:51 AM, JP Richardson notifications@github.com
wrote:

I agree that using lodash as an ideal model and I'm in favor of that.
This PR looks good as far as I can tell.


Reply to this email directly or view it on GitHub
#135 (comment)
.

@jprichardson
Copy link
Owner

My guess without looking at it ATM is that it's because count was extracted
out into its own file. It needs to be put back in string.js until we refine
our build process.

On Mon, Mar 23, 2015 at 2:55 PM, Azharul Islam notifications@github.com
wrote:

Can you take a look at the test/browser.test.html ? seems to be its broken
because of this not sure though.

On Tue, Mar 24, 2015 at 1:51 AM, JP Richardson notifications@github.com
wrote:

I agree that using lodash as an ideal model and I'm in favor of that.
This PR looks good as far as I can tell.


Reply to this email directly or view it on GitHub
<
https://github.com/jprichardson/string.js/pull/135#issuecomment-85166910>

.


Reply to this email directly or view it on GitHub
#135 (comment)
.

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com
Bitcoin / JavaScript: http://cryptocoinjs.com
Follow JP Richardson on Twitter: https://twitter.com/jprichardson

@jprichardson
Copy link
Owner

We don't have to do this now, but we should ultimately drop
browser.test.html in favor of using mochify/saucelabs.

On Mon, Mar 23, 2015 at 3:36 PM, JP Richardson jprichardson@gmail.com
wrote:

My guess without looking at it ATM is that it's because count was
extracted out into its own file. It needs to be put back in string.js until
we refine our build process.

On Mon, Mar 23, 2015 at 2:55 PM, Azharul Islam notifications@github.com
wrote:

Can you take a look at the test/browser.test.html ? seems to be its broken
because of this not sure though.

On Tue, Mar 24, 2015 at 1:51 AM, JP Richardson notifications@github.com
wrote:

I agree that using lodash as an ideal model and I'm in favor of that.
This PR looks good as far as I can tell.


Reply to this email directly or view it on GitHub
<
https://github.com/jprichardson/string.js/pull/135#issuecomment-85166910>

.


Reply to this email directly or view it on GitHub
#135 (comment)
.

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com
Bitcoin / JavaScript: http://cryptocoinjs.com
Follow JP Richardson on Twitter: https://twitter.com/jprichardson

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com
Bitcoin / JavaScript: http://cryptocoinjs.com
Follow JP Richardson on Twitter: https://twitter.com/jprichardson

@az7arul
Copy link
Collaborator Author

az7arul commented Mar 23, 2015

extracting count is not a problem here since browserify takes care of that
problem, in firefox the count tests passed but the style was broken.

On Tue, Mar 24, 2015 at 2:38 AM, JP Richardson notifications@github.com
wrote:

We don't have to do this now, but we should ultimately drop
browser.test.html in favor of using mochify/saucelabs.

On Mon, Mar 23, 2015 at 3:36 PM, JP Richardson jprichardson@gmail.com
wrote:

My guess without looking at it ATM is that it's because count was
extracted out into its own file. It needs to be put back in string.js
until
we refine our build process.

On Mon, Mar 23, 2015 at 2:55 PM, Azharul Islam <notifications@github.com

wrote:

Can you take a look at the test/browser.test.html ? seems to be its
broken
because of this not sure though.

On Tue, Mar 24, 2015 at 1:51 AM, JP Richardson <
notifications@github.com>
wrote:

I agree that using lodash as an ideal model and I'm in favor of that.
This PR looks good as far as I can tell.


Reply to this email directly or view it on GitHub
<

https://github.com/jprichardson/string.js/pull/135#issuecomment-85166910>

.


Reply to this email directly or view it on GitHub
<
https://github.com/jprichardson/string.js/pull/135#issuecomment-85167868>
.

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com
Bitcoin / JavaScript: http://cryptocoinjs.com
Follow JP Richardson on Twitter: https://twitter.com/jprichardson

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com
Bitcoin / JavaScript: http://cryptocoinjs.com
Follow JP Richardson on Twitter: https://twitter.com/jprichardson


Reply to this email directly or view it on GitHub
#135 (comment)
.

@az7arul
Copy link
Collaborator Author

az7arul commented Mar 23, 2015

will merge it after I address the Travis CI build failure

@az7arul
Copy link
Collaborator Author

az7arul commented Mar 26, 2015

@jprichardson This needs some more work.

  1. Fix the build system for node 0.8
  2. Fix the extendPrototype method since it returns an object instead of string

'hi'.capitalize();

[String: 'Hi']

  1. Change spec for restorePrototype() since endsWith is specified in ECMA6 and iojs and some browser have it as a built in method.

@jprichardson
Copy link
Owner

  1. Let's drop Node 0.8 and just stick with 0.10, 0.12, iojs.
  2. Fix it how?
  3. Sounds good to me.

@jprichardson
Copy link
Owner

I dropped 0.8: 82da0e7

@az7arul
Copy link
Collaborator Author

az7arul commented Mar 27, 2015

extendPrototype is still broken, probably it was broken before this change. I will look into it today.

@az7arul az7arul merged commit 8e67a50 into jprichardson:master May 29, 2015
@az7arul
Copy link
Collaborator Author

az7arul commented May 29, 2015

glad this is finally over

@jprichardson
Copy link
Owner

Awesome, nice work!

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.

None yet

2 participants