paldepind / union-type

A small JavaScript library for defining and using union types.
MIT License
477 stars 28 forks source link

Next releae #31

Closed paldepind closed 8 years ago

paldepind commented 8 years ago

Thanks to great ideas from @alexeygolev, help from @gilligan and inspiration from @scott-christopher's work here I'm planning to do a new breaking release. I'd really like some feedback and opinions :smile:

New features include

var Point = Type({Point: {x: Number, y: Number}});

var Shape = Type({
  Circle: {radius: Integer, center: Point},
  Rectangle: {upperLeft: Point, bottomRight: Point},
});

Fields can be accessed like normal properties. Eg. circle.radius. Records support destruction and pattern matching like regular union types.

var Maybe = Type({Just: [T], Nothing: []});
Maybe.prototype.map = function(fn) {
  return Maybe.case({
    Nothing: () => Maybe.Nothing(),
    Just: (v) => Maybe.Just(fn(v))
  }, this);
};
var just = Maybe.Just(1);
var nothing = Maybe.Nothing();
nothing.map(add(1)); // => Nothing
just.map(add(1)); // => Just(2)

I'm also considering deprecating switch and switchOn and renaming them to match and matchOn. If anyone has a better name than matchOn I'd love to hear it.

davidchambers commented 8 years ago

I spotted a typo in the title: s/releae/release/.

davidchambers commented 8 years ago

Rather than use process.env.NODE_ENV !== 'production', I'd like to be explicit about whether type checking is enabled. In sanctuary-def, $.create takes checkTypes :: Boolean as its first argument. Users are free to provide process.env.NODE_ENV !== 'production', but they're not forced to overload NODE_ENV, nor must they polyfill process in the browser.

gilligan commented 8 years ago

I would also like to see the process.env part go in favor of something more explicit! I would even prefer no switch at all just as long as some environment variable does not change the behavior of some library somewhere in my stack.

gilligan commented 8 years ago

my vim complains about mixed indentation hehe. @paldepind I wonder if you would be open for a PR adding linting via eslint for the sake of consistency and error checking. We could use the same settings that I introduced to Ramda: https://github.com/ramda/ramda/pull/1529

paldepind commented 8 years ago

@davidchambers

Rather than use process.env.NODE_ENV !== 'production', I'd like to be explicit about whether type checking is enabled. In sanctuary-def, $.create takes checkTypes :: Boolean as its first argument. Users are free to provide process.env.NODE_ENV !== 'production', but they're not forced to overload NODE_ENV, nor must they polyfill process in the browser.

I very much agree. But one advantage of using process.env.NODE_ENV is that one can do a build using something like envify which will replace the environment variable with a plain literal and then a minifier can completely remove all the error checking code. Webpack can do something similar as well. If you set a variable to process.env.NODE_ENV then a minifier will not be able to remove the code. I'm not sure what the best approach is here. I can imagine that many people would like the type checking to be present even in production code.

@gilligan

my vim complains about mixed indentation hehe. @paldepind I wonder if you would be open for a PR adding linting via eslint for the sake of consistency and error checking. We could use the same settings that I introduced to Ramda: ramda/ramda#1529

It's the darn Emacs. Using linting would be great.

kwijibo commented 8 years ago

I'm also considering deprecating switch and switchOn and renaming them to match and matchOn. If anyone has a better name than matchOn I'd love to hear it.

@paldepind Do you mean case and caseOn ? I like match, but not sure about matchOn.. perhaps matchWith? (although that also suggests that the extra param is for the matching, and not the execution of the matched function ...)

paldepind commented 8 years ago

@paldepind Do you mean case and caseOn ? I like match, but not sure about matchOn.. perhaps matchWith? (although that also suggests that the extra param is for the matching, and not the execution of the matched function ...)

Yes. Indeed. I think match is nice and it would be better to avoid using a keyword. I agree that matchOn is ugly. caseOn is as well I think. I do think matchWith sounds better. But you're right that it implies that the parameter is part of the matching. But caseOn did that that as well.

kwijibo commented 8 years ago

@paldepind if you match a Record, what gets passed to the function? the record, or each value as an argument (like with the typed arrays)? Is it like Postal: ({name,address, postalcode}) => or like Postal: (name, address,postalcode) => ?

paldepind commented 8 years ago

@kwijibo The latter, each value as argument.

kwijibo commented 8 years ago

Thanks. I'm looking forward to this release. I started using union-type recently, and am really liking the more domain-driven, declarative style it affords.

Glad to see the prototype stuff; presumably Action.Up() instanceof Action will work now?

kwijibo commented 8 years ago

({name,address, postalcode}) would be a bit nicer from the POV of not needing to look up the order the fields were defined in.

paldepind commented 8 years ago

That is a good point @kwijibo. On the other hand it would not be as nice for people not using ECMAScript 6.

I'm glad you enjoy the library.

gilligan commented 8 years ago

Anything I can contribute to move this forward ? The one thing that still bugs me is the use of NODE_ENV because it just isn't explicit at all - you have some big project which as some sub-dependency uses union-type and it changes behaviour like that based on an environment variable - I don't like that ;(

As for minifying - the code is so small already I don't care about those few bytes more or less and value the code being correct and behaviour explicit.

My suggestion would be to either remove the ability to switch checks on or off completely or follow the suggestion made by @davidchambers to pass in a predicate function.

All other changes are probably minor and could thus be followed as minor version updates (glad to provide an eslint PR later on for example).

paldepind commented 8 years ago

Merged and released. Type checking is disabled by doing Type.check = false.