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

Temporarily revert "Add exact type checker from react native" #80

Merged
merged 1 commit into from Jun 15, 2017

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jun 15, 2017

Reverts #41

cc @aweary

There were two issues that were missed during review:

Prod version

See this comment. Lists in these two files must be in sync. This is essential because as far as I can see PropTypes.exact({}).isRequired will throw in production code.

We need to automate this check because I agree it's not obvious.

Object.assign

I don't think this repo is set up to polyfill Object.assign yet, so this will throw in older browsers.

Way forward

@aweary Can you fix those two issues? Happy to take the PR back in after that.

For now I have to revert because I don't have time to fix forward and I'm worried we'll forget about this, and ship a broken version.

@ljharb
Copy link
Collaborator

ljharb commented Jun 15, 2017

object.assign is a great non-auto-shimming polyfill if you need one :-)

@gaearon
Copy link
Contributor Author

gaearon commented Jun 15, 2017

We're using object-assign in React repo so would make sense for consistency.
We should really set up Babel here to match our transforms..

@ljharb
Copy link
Collaborator

ljharb commented Jun 15, 2017

(you should use object.assign there too :-p)

Regardless of which module you use, you should use https://www.npmjs.com/package/babel-plugin-transform-replace-object-assign so that babel correctly transpiles object spread and Object.assign to use the module.

@aweary
Copy link
Contributor

aweary commented Jun 15, 2017

@gaearon sorry about that, I wasn't aware of either of those conditions ☹️ I can fix, but not until next week unfortunately.

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

Successfully merging this pull request may close these issues.

None yet

4 participants