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

add mapContains prop type #24

Merged
merged 2 commits into from
Feb 20, 2016
Merged

Conversation

hartzis
Copy link
Contributor

@hartzis hartzis commented Feb 12, 2016

attempt to add functionality for #15

Example data:

Immutable.fromJS({
    name: 'bobiton',
    nickname: 'thebob',
    pictures: ['/bobswimming.jpg', 'bobflying.jpg']
})

Example proptypes:

static propTypes = {
    user: ImmutablePropTypes.mapContains({
        name: PropTypes.string.isRequired,
        nickname: PropTypes.string.isRequired,
        pictures: ImmutablePropTypes.listOf(PropTypes.string.isRequired)
    })
};

please give feedback. maybe a different name? mapShapeOf, mapHasShape

I will update the docs when/if this functionality is agreed upon.

@MatthewHerbst
Copy link

I don't think this is related to #15 . That issue deals with validating the type and/or format of the keys themselves. That is useful when you don't actually know what they keys will be, but you do know what type/format they need to be.

It's already possible without this pr to validate the name of keys in a map and the types of the values of each key.

@hartzis
Copy link
Contributor Author

hartzis commented Feb 12, 2016

@MatthewHerbst I believe you might be thinking of #4 maybe?

from @indeyets in #15

I want to validate that something is an immutable Map with specific keys, which have values of specific types.

I believe this PR is a solution specific to that use case.

@MatthewHerbst
Copy link

@hartzis oh yes, very sorry - too many issues to keep track of! 🎱

@HurricaneJames
Copy link
Owner

What would you think about naming it mapContains?

@hartzis
Copy link
Contributor Author

hartzis commented Feb 12, 2016

i like mapContains, seems a little more explicit.

);
}
var mutablePropValue = propValue.toJS();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This morning I realized I should probably convert this back to .toObject. I hadn't realized before today that it only shallowly converts. Which allows you to continue to propType check deeply for Immutable objects.

Immutable.fromJS({a: [1, 2]}).toObject() => { a: List [ 1, 2 ] }
Immutable.fromJS({a: [1, 2]}).toJS() => { a: [ 1, 2 ] }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.toObject allows for:

mapContains({
    id: PropTypes.string.isRequired,
    list: listOf(mapContains({
        val: PropTypes.number.isRequired,
    })).isRequired,
})

@hartzis hartzis changed the title add mapShape prop type add mapContains prop type Feb 13, 2016
@hartzis
Copy link
Contributor Author

hartzis commented Feb 13, 2016

Updated the PR:

  • changed to mapContains
  • revert back to using .toObject to allow nested immutable propType checking
    • add test for nested immutable propType checking
  • update readme with mapContains, and add example
  • remove confusing contains example testing a List

@hartzis
Copy link
Contributor Author

hartzis commented Feb 14, 2016

@HurricaneJames looks like you accidentally wrote mapContains in the 1.6.0 changelog instead of stackOf

let me know if you see anything else with this PR, and I'll get it updated asap. Cheers.

@HurricaneJames HurricaneJames merged commit 422838f into HurricaneJames:master Feb 20, 2016
@HurricaneJames
Copy link
Owner

merged, made a couple changes, and published v1.7.0 to npm.

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.

3 participants