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

Move unnecessary normal dependencies to devDependencies #168

Closed
wants to merge 1 commit into from

Conversation

rpominov
Copy link
Member

These dependencies are not needed for fantasy-land npm package to work.

@davidchambers
Copy link
Member

These dependencies are not needed for fantasy-land npm package to work.

That depends on which parts of the package you're using. require('fantasy-land') works without daggy and fantasy-combinators, but require('fantasy-land/laws/traversable') requires these dependencies to be installed.

Are you suggesting the laws should be defined in a separate npm package, or that since they're only used for testing they should only not be installed in production?

@rpominov
Copy link
Member Author

Ah, good point. I forgot that package also exposes laws tests. Not sure what a right course of action here then. I'll close this PR for now, maybe we'll figure out something later.

@rpominov rpominov closed this Sep 18, 2016
@safareli
Copy link
Member

Use of duggy in laws/traversable is sort of redundant in think, we could replace that lines with this:

-const {tagged} = require('daggy');
-const Compose = tagged('c');
+function Compose(c) {
+  if (!(this instanceof Compose)) {
+    return new Compose(c)
+  }
+  this.c = c
+}

(actually same is in id_test too: const Sum = tagged('v');)

And about fantasy-combinators we use Identity (12 times), apply (3 times) and compose (3 times). there are 9 functions in fantasy-combinators and we use 3 and each is just one line, so maybe it's better to could just create combinators.js and include this 3 functions in there?

@SimonRichardson
Copy link
Member

I'm not a fan of code duplication for the sake of it :|

On Thu, 22 Sep 2016, 10:23 Irakli Safareli, notifications@github.com
wrote:

Use of duggy in laws/traversable is sort of redundant in think, we could
replace that lines with this:

-const {tagged} = require('daggy');-const Compose = tagged('c');+function Compose(c) {+ if (!(this instanceof Compose)) {+ return new Compose(c)+ }+ this.c = c+}

(actually same is in id_test too: const Sum = tagged('v');)

And about fantasy-combinators we use Identity (12 times), apply (3 times)
and compose (3 times). there are 9 functions in fantasy-combinators and
we use 3 and each is just one line, so maybe it's better to could just
create combinators.js and include this 3 functions in there?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#168 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACcaGEN0FAc0CC22X3nOidgfiBNh8B85ks5qskkGgaJpZM4J_5xT
.

@safareli
Copy link
Member

safareli commented Sep 22, 2016

On one side a) we have 2 dependencies and on other side b) duplication of 3 online functions and expansion of 2 constructors.

If we go with b) FL will have 0 dependency so library contributors will be easily convinced to use FL as dependency. Of course this point has value in case we want libs to depend on FL, which I think is what we should be aiming for as it will make it clear what version of FL a lib conforms to without digging into it's docs.

@rpominov
Copy link
Member Author

Good point about determining to which version of FL a library conforms to. Didn't thought about it this way.

@safareli
Copy link
Member

Also it's handy to see libs which depend on FL in npm

@rpominov
Copy link
Member Author

Wow, that's a good alternative to implementations.md that can be more complete and automatically up to date!

@SimonRichardson
Copy link
Member

SimonRichardson commented Sep 22, 2016

I think if it boils down to this it's beyond pathetic. I just don't see value in it. If we think this, then I'd go the whole hog and remove all the other libraries in fantasy-land repos and just host a markdown file.

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.

4 participants