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

Next releae #31

Merged
merged 25 commits into from
Mar 2, 2016
Merged

Next releae #31

merged 25 commits into from
Mar 2, 2016

Conversation

paldepind
Copy link
Owner

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 😄

New features include

  • Implementation is based on prototypes, should give better performance
  • All type checks are disabled in production builds. Type checking code can be completely "compiled away" for browser builds.
  • Records are now supported with the following syntax:
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.

  • Instance methods can be created like below. This for instance allow for implementation of ADTs that support fantasy land.
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
Copy link
Contributor

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

@davidchambers
Copy link
Contributor

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
Copy link
Contributor

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.

function isFunction(f) { return typeof f === 'function'; }
var isArray = Array.isArray || function(a) { return 'length' in a; };
if (process.env.NODE_ENV !== 'production') {
var isString = function(s) { return typeof s === 'string'; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason to not use R.is ?

var is = require('ramda/src/is');

var isString = R.is(String);
var isNumber = R.is(Number);
...

either that or just use R.is directly.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason is to keep the external dependencies as low as possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that, but Ramda is already a dependency anyway ;)

@gilligan
Copy link
Contributor

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

};
}

function valueToArray(value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly lost the ability the parse for loops without getting a headache .. How about:

const valueToArray = (value) => value.keys.map((x, index) => value[value.keys[index]]);

: undefined;
if (name === undefined) {
throw new Error('unhandled value passed to case');
function extractValues(keys, obj) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I would probably prefer to use map and es6 syntax here:

const extractValues = (keys, obj) => keys.map((x, index) => obj[keys[index]]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we are not using es6:

function extractValues (keys, obj) {
  return keys.map(function (x, index) {
    return obj[keys[index]];
 });
};

Admittedly not as nice ;) Maybe we could add es6 transpiling at some point ;)

@paldepind
Copy link
Owner Author

@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
Copy link

kwijibo commented Feb 26, 2016

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
Copy link
Owner Author

@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
Copy link

kwijibo commented Feb 26, 2016

@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
Copy link
Owner Author

@kwijibo The latter, each value as argument.

@kwijibo
Copy link

kwijibo commented Feb 26, 2016

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
Copy link

kwijibo commented Feb 26, 2016

({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
Copy link
Owner Author

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
Copy link
Contributor

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 added a commit that referenced this pull request Mar 2, 2016
@paldepind paldepind merged commit 1103d2d into master Mar 2, 2016
@paldepind
Copy link
Owner Author

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

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

4 participants