search

codemix / babel-plugin-typecheck

Static and runtime type checking for JavaScript in the form of a Babel plugin.
MIT License
886 stars 44 forks source link

Support `import type {Foo}` where Foo is a concrete class. #112

Closed panrafal closed 7 years ago

panrafal commented 8 years ago

Since upgrading babel to 6.5.x, I've found two issues with code which compiled without problems before.

Babel-cli is at version 6.5.1, babel-core at 6.5.2 Node at 5.4.1 (happens at 5.6.0 too)

Both examples fail when built using babel test...js. You can find the test project here: https://github.com/panrafal/babel-typecheck-test

Quote after opening brace of object type

const foo = (query: { // eslint-disable-line
  data: string
}) => {
  return data
}

Results in:

TypeError: test-object-quote.js: Cannot read property 'start' of undefined
    at /Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/whitespace.js:29:19
    at Whitespace._findToken (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/whitespace.js:99:30)
    at Whitespace.getNewlinesBefore (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/whitespace.js:28:22)
    at CodeGenerator.printComment (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/printer.js:321:34)
    at CodeGenerator.printComments (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/printer.js:377:12)
    at CodeGenerator.printLeadingComments (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/printer.js:231:10)
    at CodeGenerator.print (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/printer.js:81:10)
    at CodeGenerator.printJoin (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/printer.js:193:12)
    at CodeGenerator.ObjectTypeAnnotation (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/generators/flow.js:266:10)
    at CodeGenerator._print (/Users/rafal/github/typecheck-test/node_modules/babel-generator/lib/printer.js:155:17)

Import type declaration

import type foo from './foo'

Results in:

TypeError: test-import-type.js: Property imported of ImportSpecifier expected node to be of a type ["Identifier"] but instead got null
    at Object.validate (/Users/rafal/.nvm/versions/node/v5.4.1/lib/node_modules/babel-cli/node_modules/babel-types/lib/definitions/index.js:101:13)
    at validate (/Users/rafal/.nvm/versions/node/v5.4.1/lib/node_modules/babel-cli/node_modules/babel-types/lib/index.js:269:9)
    at Object.builder (/Users/rafal/.nvm/versions/node/v5.4.1/lib/node_modules/babel-cli/node_modules/babel-types/lib/index.js:222:7)
    at /Users/rafal/github/typecheck-test/node_modules/babel-plugin-typecheck/lib/index.js:99:29
    at Array.map (native)
    at Object.ImportDeclaration (/Users/rafal/github/typecheck-test/node_modules/babel-plugin-typecheck/lib/index.js:96:57)
    at NodePath._call (/Users/rafal/.nvm/versions/node/v5.4.1/lib/node_modules/babel-cli/node_modules/babel-traverse/lib/path/context.js:74:18)
    at NodePath.call (/Users/rafal/.nvm/versions/node/v5.4.1/lib/node_modules/babel-cli/node_modules/babel-traverse/lib/path/context.js:46:17)
    at NodePath.visit (/Users/rafal/.nvm/versions/node/v5.4.1/lib/node_modules/babel-cli/node_modules/babel-traverse/lib/path/context.js:104:12)
    at TraversalContext.visitQueue (/Users/rafal/.nvm/versions/node/v5.4.1/lib/node_modules/babel-cli/node_modules/babel-traverse/lib/context.js:156:16)
phpnode commented 8 years ago

@panrafal please can you paste your full .babelrc ?

panrafal commented 8 years ago

@phpnode it's in the project: https://github.com/panrafal/babel-typecheck-test/blob/master/.babelrc

Our original project is huge, but I've distilled the problems to these two test files which fail every time. I can run all typecheck's tests without problems but trying to compile these files in the same directory also fails.

phpnode commented 8 years ago

Ok this is two separate issues:

1 - The import test doesn't work because we don't support default imports/exports for types. If you change the statement to 

import type {wtf} from './wtf'

it works. I'm not sure whether flow supports this, I'll investigate before v4.

2 - The second example is actually failing because of a bug in babel-generator - if you remove the comment it works.

panrafal commented 8 years ago

@phpnode Thanks for the quick reply.

1) It worked before and according to this blog post its supported in flow: http://flowtype.org/blog/2015/02/18/Import-Types.html

Another problem I have now, is that after using a named import, the type is checked by calling it, instead of using instanceof. Most of imported types are classes. You have an unreleased version that I believe should fix that. Is there anything holding you back from the release?

2) With the complexity of Babel ecosystem blaiming is hard :) This can be fixed by declaring babel-generator: '6.4.5' in package.json. there is an unmerged PR fixing this already.

phpnode commented 8 years ago

@panrafal because we can't do whole program analysis (this was a planned feature of babel 6 that didn't make it to release) we have no way of telling what flavour of entity you're importing when you import type. Therefore we treat it just like a type you've declared. This is definitely not ideal.

For v4 I want to really focus on compatibility with flow and this is an example of something that is hard to do within the confines of a babel plugin as it stands. It's possible that in order to solve this, we have to stop being a babel plugin and run our own compilation passes.

panrafal commented 8 years ago

@phpnode But if every typecheck would use instanceof, wouldn't that actually solve it? As long as you have babel-instanceof enabled, you don't have to know if it's a class, or a type declaration - you can just use instanceof.

As a workaround for now, I understand that I can do this kind of stuff (it compiles, looks like it should work):

// a.js
export default class Foo {}
export type FooClass = Foo
// b.js
import type {FooClass} from './a'
function foo(param: FooClass) {}
phpnode commented 8 years ago

@panrafal in the past I did use that Symbol.hasInstance approach but ran into issues with babel's plugin ordering, the new passPerPreset option might fix that.

phpnode commented 7 years ago

Hi, sorry for taking so long to respond to this, this project is now deprecated in favour of https://codemix.github.io/flow-runtime which aims for full compatibility with Flow.

babel-plugin-flow-runtime does not have this particular bug because we can tell the difference between types and values - types are instanceof Type.