Skip to content
This repository has been archived by the owner on Feb 16, 2021. It is now read-only.

$Refinement doesn't catch this case #78

Closed
volkanunsal opened this issue Jul 29, 2016 · 6 comments
Closed

$Refinement doesn't catch this case #78

volkanunsal opened this issue Jul 29, 2016 · 6 comments

Comments

@volkanunsal
Copy link

volkanunsal commented Jul 29, 2016

I have the following type definition

const isPoly = layer => layer instanceof L.Polyline || layer instanceof L.Polygon || layer instanceof L.Rectangle
type PolyT = Object & $Refinement<typeof isPoly>

And I use it this way:

getLayer(e: Object) {
    let ret: PolyT = e.layer || e.target || e   // <= Doesn't throw
    return ret
}

addLayer(e) {
  let layer = this.getLayer()
 if(!isPoly(layer)) { throw new Error('Layer is not a poly') }  // <= Throws
}

As it will become clear, the type definition doesn't throw, even though the refinement function returns false.

The compiled assertion function for this is the following:

function _assert2(x, type, name) {
  if (_tcomb5.default.isType(type) && type.meta.kind !== 'struct') {
    type(x, [name + ': ' + _tcomb5.default.getTypeName(type)]);
  } else if (!(x instanceof type)) {
    _tcomb5.default.fail('Invalid value ' + _tcomb5.default.stringify(x) + ' supplied to ' + name + ' (expected a ' + _tcomb5.default.getTypeName(type) + ')');
  }

  return x;
}

Also, it may be relevant that this function is defined twice in the compiled code. The same is true for the calls.

function getLayer(e) {
      _assert2(e, _tcomb3.default.Object, 'e');

      _assert2(e, _tcomb3.default.Object, 'e');


      var ret = e.layer || e.target || e;
      return ret;
}

Btw, I just noticed that it looks like the refinement type is not asserted in the function body at all actually. That seems to be the real issue here.

@gcanti
Copy link
Owner

gcanti commented Jul 29, 2016

Hi @volkanunsal, thanks for the detailed issue. Some observations:

lets are not supported, use consts instead

getLayer(e: Object) {
  const ret = PolyT = e.layer || e.target || e   // <= const
  return ret
}

You might even use a type cast in this case

getLayer(e: Object) {
  return (e.layer || e.target || e: PolyT) // <= type cast
}

or a typed return value

getLayer(e: Object): PolyT { // <= return value
  return e.layer || e.target || e
}

Also, it may be relevant that this function is defined twice in the compiled code

Make sure you added "passPerPreset": true in your babel configuration (#53)

@gcanti
Copy link
Owner

gcanti commented Jul 29, 2016

Also, since your predicate uses only instanceof you can define PolyT as a union

type PolyT = L.Polyline | L.Polygon | L.Rectangle;

@volkanunsal
Copy link
Author

volkanunsal commented Jul 29, 2016

@gcanti I tried, but I couldn't get the return type to work. Typecasting does work. Also here is my .babelrc file. I do have the passPerPreset, it seems.

{
  "presets": ["es2015", "react", "stage-0"],
  "passPerPreset": true,
  "plugins": [
    "tcomb",
    "transform-decorators-legacy",
    "transform-class-properties",
    "syntax-flow",
    "transform-flow-strip-types"
  ]
}

Also, I'm getting SyntaxError: Unexpected token when I try to use const

 const ret: PolyT = e.layer || e.target || e

@volkanunsal
Copy link
Author

volkanunsal commented Jul 29, 2016

Oh, sorry, I didn't have passPerPreset actually. When I added it just now, I'm getting all sorts of errors from Babel:

Module build failed: TypeError: Property name of JSXOpeningElement expected node to be of a type ["JSXIdentifier","JSXMemberExpression"] but instead got "MemberExpression"

Maybe related to this issue.

@volkanunsal
Copy link
Author

volkanunsal commented Jul 29, 2016

@gcanti I fixed the issue! The problem is if you use passPerPreset and use multiple presets, as I do, you need to specify them in just the right order. Otherwise, Babel will freak out. Here is the order that worked for me:

presets: ['stage-0', 'react', 'es2015'],

@gcanti
Copy link
Owner

gcanti commented Jul 29, 2016

you need to specify them in just the right order

Ah, thanks for explaining. I'll add that to the docs.

p.s.
"syntax-flow" and "transform-flow-strip-types" plugins seem redundant as you are using the "react" preset

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants