search

pbomb / flow-immutable-models

Generates model classes from Flow types using Immutable.js
42 stars 8 forks source link

Detect when ModelType is erroneously applied to union type #19

Closed rgbkrk closed 7 years ago

rgbkrk commented 7 years ago

I've been experimenting with using this codemod on nteract's core notebook model (which uses commutable underneath).

The base repo I've set up for experimenting with is https://github.com/nteract/commutable-models, which you can try out with flow-immutable-models by running:

git clone https://github.com/nteract/commutable-models
cd commutable-models
npm i
npm run generate:models

I was getting this error

> commutable-models@1.0.0 generate:models /Users/kylek/code/src/github.com/nteract/commutable-models
> jscodeshift -t ./node_modules/flow-immutable-models/lib/transform.js ./src/models.js

Processing 1 files...
Spawning 1 workers...
Sending 1 files to free worker...
 ERR ./src/models.js Transformation error
TypeError: Cannot read property 'filter' of undefined
    at fromJS (/Users/kylek/code/src/github.com/nteract/commutable-models/node_modules/flow-immutable-models/lib/helpers/fromJS.js:66:57)
    at makeClass (/Users/kylek/code/src/github.com/nteract/commutable-models/node_modules/flow-immutable-models/lib/transform.js:21:47)
    at NodePath.<anonymous> (/Users/kylek/code/src/github.com/nteract/commutable-models/node_modules/flow-immutable-models/lib/transform.js:81:17)
    at __paths.forEach (/Users/kylek/code/src/github.com/nteract/commutable-models/node_modules/jscodeshift/dist/Collection.js:76:36)
    at Array.forEach (native)
    at Collection.forEach (/Users/kylek/code/src/github.com/nteract/commutable-models/node_modules/jscodeshift/dist/Collection.js:75:18)
    at exports.default (/Users/kylek/code/src/github.com/nteract/commutable-models/node_modules/flow-immutable-models/lib/transform.js:68:6)
All done.
Results:
1 errors
0 unmodified
0 skipped
0 ok

Which was a result of erroneously putting the ModelType suffix on a union type like this:

export type CellModelType = CodeCellModelType | MarkdownCellModelType

when it should have been

export type Cell = CodeCellModelType | MarkdownCellModelType

PR that fixed it: https://github.com/nteract/commutable-models/pull/2

At least for development experience, it would be nice if this package could detect this type of behavior. Right now, if propExpressions is undefined in fromJS it's likely that this is the case.

pbomb commented 7 years ago

Interesting. I should be able to add support for static literal strings quickly. And the Union type is interesting as it presents some challenges for the generated code.

I need to add better documentation, but the idea is that if a property references a collection of another model, then the flow type would look like:

export type FooModelType = {
  prop: Array<BarModelType>,
};

The library would convert that to a Immutable.List<Bar> for you. So, you'd pass built-in js objects and they'd be converted to Immutable and the model instances for you. Union types might struggle with that because we wouldn't know which model to create. You might have to work around that if you need unions of other model types.

On Fri, Jan 13, 2017, 3:05 PM Kyle Kelley notifications@github.com wrote:

Ok, figured it out by modifying source directly in node_modules - I was putting the ModelType suffix on union types (when they didn't need it). I can reword this issue.

nteract/commutable-models#2 https://github.com/nteract/commutable-models/pull/2

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pbomb/flow-immutable-models/issues/19#issuecomment-272560709, or mute the thread https://github.com/notifications/unsubscribe-auth/ABVk7yxpTvs-SkHG5cn4Tu6ZpcM0ndjGks5rR_UsgaJpZM4LjQXR .

pbomb commented 7 years ago

@rgbkrk how would you feel if the following error message had been presented to you. Is it enough? Or does it use too much unfamiliar terminology? Expected CellModelType to be of type ObjectTypeAnnotation. Instead it was of type UnionTypeAnnotation.

rgbkrk commented 7 years ago

That's perfect, I certainly would have figured it out faster without having to hack on flow-immutable-models directly.

If there's room to add another message to the TypeError, I'd probably follow the exact message you have with something to help the user fix their own code:

Expected CellModelType to be of type ObjectTypeAnnotation. Instead it was of type UnionTypeAnnotation.

If you meant to use a Union type for "Cell", you don't have to declare it as a ModelType.

That's bonus points level though. :smile: