xodio/hm-def

Update deps

danielo515 opened this issue · 18 comments

danielo515 commented

Hello,
I noticed that you use several dependencies, like Ramda, and Ramda-fantasyland.

Ramda fantasyland has been recently deprecated, and since you are targeting sanctuary already, why not just using sanctuary ?

Regards

danielo515 commented

This function is sadly not working with the latest version of sanctuary neither.

If you annotate a function this is what you get when the signature does not match:

joinPath :: [object Object] -> [object Object] -> [object Object] -> [object Object]

When the actual signature is just String -> String -> String -> String

And if the types match, it tries to call the function wrong, because now sanctuary functions are all unary

  • 👍1
rockymadden commented

I can confirm @danielo515's observations.

danielo515 commented

I took a look to see if I can make a PR but I'm having problems just trying to use sanctuary def for my own functions.
I can't even find the usage of sanctuary def inside this repository

nkrkv commented

Hello!

Can you clarify a bit? Are you trying to bump dependencies just for the sake of update or are you trying to bring some features that not possible at the current dependency versions?

danielo515 commented

Sorry for being unclear.
At first it was just for the shake of being up to date. However with the latest release of sanctuary this has stopped working, hence the need to update the code to make it work with the latest version of sanctuary, which includes several improvements.

Regards

rockymadden commented

This is actually a more complex problem than first appears:

  1. Recent changes in sanctuary, sanctuary-def, and sanctuary-type-classes has moved to "familiar currying" (https://sanctuary.js.org/#currying). As a result, we need to adopt this usage style. I've done this and it's pretty easy.
  2. Use sanctuary-def as a peer dependency. Force the user to provide their own sanctuary-def within a range of something hm-def supports, rather than only supporting some static version this library is using which may or may not match up with what they are using. As long as the API for what hm-def interfaces with remains the same, it should be okay to allow the user to use whatever. If we use the "familiar currying" style within hm-def, this should support a wider range for versions (since older versions of sanctuary-def should work as well). I've done this and it is also easy.
  3. Handle for every where within the codebase where arity is detected and is required, for one reason or another. This appears to be far more difficult. For instance, what is the arity of FutureType when you cannot tell via something like fn.length due to it adopting the familiar currying style?

I'm still plugging away, but I thought I'd mention here to follow back up.

rockymadden commented

Upon reflection, this might be better served as a separate/different GH issue. It IS what @danielo515 mentions in a later comment, but it does not match the issue title. I'm ignoring the Ramda/Ramda-Fantasy aspects. Instead, I am focusing on:

  • Supporting current versions of sanctuary-def and other sanctuary-def related libraries
  • Supporting the idea of allowing for a range of sanctuary-def versions to work via peer dependencies
  • Perhaps moving to the "familiar currying" style as well (e.g. dropping def in favor of def.curried)

The end result would be BYOSD and then usage like so:

const add = def
  ('add :: Number -> Number -> Number')
  (a => b => a + b);
  • 👍2
davidchambers commented

@rockymadden, I suggest opening a pull request for the work you've done so far. It'll then be easier for others to work with you to overcome the remaining hurdles.

Gipphe commented

So, I've been having a crack at this, and I've got something that works with sanctuary-def@>=0.15.0. I kinda went overboard, and might have to break this into multiple PRs, if having massive multi-feat PRs isn't desired:

  • Compatible with the "familiar currying" (also known as "manual currying") style of sanctuary. No longer compatible with non-curried function application. It was simpler this way, and this is a hard requirement even sanctuary has, so I felt justified enforcing this requirement here as well.
  • I went rather overboard, and replaced ramda in its entirety with sanctuary, some choice functions from lodash, and mem for memoization.
  • I also updated to babel version 7, and swapped to @babel/preset-env since babel is doing away with the stage presets.
  • Replaced ramda-fantasy and its Reader with a tweaked implementation Reader that is fantasy-land compatible. Might be worthwhile to extract it into its own package. As far as I can tell, fantasy-readers seems pretty dead. I'm currently writing some law tests this Reader.
  • Updated mocha to 5.2.0.
  • Updated chai to 4.2.0.
  • Update the eslint-config-airbnb-base to 13.1.0, and updated its peer dependencies to match.

So, two questions:

  1. Should I break these changes up into their own PRs?
  2. Somewhat offtopic, but why do no sanctuary projects ever adhere to semver? Breaking changes end up just incrementing the minor version number, as far as I can tell. Or is it just that we do not consider sanctuary and sanctuary-def to be production-ready?

Edit:

I see now that I also did something as egregious as introduce sanctuary-style at some point in the past, and completely forgot about it until now. I guess being consistent with sanctuary is a nice thing, but this issue/potential PR is probably not the place for this kind of change, I presume.

  • ❤️2
danielo515 commented

Wow that looks cool @Gipphe !
This is of course just my opinion, because I'm not the owner of the package. By any means I try to say that you should take in account anything I will say, this are just suggestions and points of view.

I'm totally fine with the first breaking change, if you are going to use sanctuary and family you should go with the curried way of doing things.
I really hate babel v7, but if that is the future, then go for it.

Regarding PRs.. I understand that it may be easier to rollback a big change like this and locate it if it is just one single PR. Do you think there is something that can be useful for the current version ? It is working as it is, and updating it with your changes will not change the fact that it is pretty outdated.

Somewhat offtopic, but why do no sanctuary projects ever adhere to semver? Breaking changes end up just incrementing the minor version number, as far as I can tell. Or is it just that we do not consider sanctuary and sanctuary-def to be production-ready?

This is something I ask myself too, but seems to be related with the first 0 on the semver range. 0.x.x is not considered production ready and the "responsibility" is somewhat shifted from right to left, so the middle number means breaking changes. Other projects do it the same way, for example elm, with tons of breaking changes from 0.18 to 0.19 or Purescript, from 0.11 to 0.12

  • 👍1
davidchambers commented

Somewhat offtopic, but why do no sanctuary projects ever adhere to semver?

We do adhere to SemVer. sanctuary-type-classes is at v9.0.0. It's true that sanctuary and sanctuary-def have not yet reached v1.0.0, but this means breaking changes are permitted:

  1. Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

As a rule, we never make breaking changes in patch releases, even for projects which have not yet reached v1.0.0.

  • 👍2
nkrkv commented

@Gipphe Thank you for the effort! Looks cool and we are interested in applying the changes to master. It does not matter much whether it is a single PR or many. What does matter, is the backward compatibility.

hm-def is a project created for very practical purposes and it is used extensively in https://github.com/xodio/xod codebase. It is problematic to quickly and carefully switch to the new (and fine, I like it) curried style of function calling everywhere in XOD. Maybe we can provide some kind of “legacy” module to keep the current code almost intact. We need some time to perform the research.

danielo515 commented

You can also publish the new version under a namespace

Gipphe commented

@nkrkv Currently, the breaking changes are the currying style (of course), as well as requiring sanctuary-def to be passed in to create (forgot to mention that earlier). I assume passing sanctuary-def in is still desired? I see what you mean by possibly having to redo all call sites and function definitions. I'll have a look and see if I can't make a createLegacy or something of the like that supports the old-fashioned style of adaptive currying.

@danielo515 Could you elaborate on that? You mean to post it under a namespace like @Gipphe/hm-def so that people have the choice of either that one or the legacy hm-def?

danielo515 commented

You mean to post it under a namespace like @Gipphe/hm-def so that people have the choice of either that one or the legacy hm-def?

Exactly that

  • 👍1
Gipphe commented

@nkrkv The more sensible option here, if you ask me, would be to publish it under a namespace or as a separate package, if just a version bump is not adequate. To be honest, createLegacy would be simplest implemented by just having hm-def@0.3.x as a dependency and exposing that as createLegacy. Which does indeed work, don't get me wrong, but it seems unnecessary given how dependencies and versions work.

nkrkv commented

🤔 Indeed, I'm overcomplicating things. We can freeze the version on the XOD side for now. OK, let’s make the update. Would you create a PR?

Gipphe commented

Will do.