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 validation to prop options #52

Merged
merged 2 commits into from Mar 2, 2018

Conversation

Projects
None yet
3 participants
@BrianMehrman
Copy link
Contributor

BrianMehrman commented Jan 31, 2018

First pass at adding some code to let the user know the option they passed for creating a prop is not going to be used.

Throwing an error might be too heavy handed.

@BrianMehrman BrianMehrman requested a review from burrows Jan 31, 2018

@@ -140,6 +140,16 @@ function flush() {
while ((f = delayPostFlushCallbacks.shift())) { f(); }
}

var VALID_OPTIONS = ['attr', 'converter', 'name', 'get', 'set', 'default', 'on', 'cache', 'pure'];

This comment has been minimized.

@burrows

burrows Jan 31, 2018

Member

Lets make this a const.

// Internal: Throws error if option is passed that is not used.
function validateOptions(opts) {
Object.getOwnPropertyNames(opts).forEach(function(opt) {
if(VALID_OPTIONS.indexOf(opt) < 0) {

This comment has been minimized.

@burrows

burrows Jan 31, 2018

Member

The O(n) operation here bugs me a little bit. This function gets called a lot when the code is initially loaded, so this could potentially have a performance impact. We could instead make VALID_OPTIONS an object so it would just be an O(1) lookup on each option.

@burrows

This comment has been minimized.

Copy link
Member

burrows commented Jan 31, 2018

@BrianMehrman Thanks for the PR! Just a couple of minor things to address before we merge this.

@@ -192,6 +202,8 @@ function getCached(name) {
function defineProp(object, name) {
var opts = arguments.length > 2 && arguments[2] !== undefined ? arguments[2] : {};

validateOptions(opts);

This comment has been minimized.

@alexraginskiy

alexraginskiy Jan 31, 2018

Contributor

This might be a good thing to only check in non-production, non-minified builds. Would have to check the webpack setup, but typically you would use:
if (process.env.NODE_ENV !== "production") and provide that value via webpack, and minification JS would strip that code post build if it results to a definitely false predicate: "production" !== "production"

This comment has been minimized.

@BrianMehrman

BrianMehrman Jan 31, 2018

Contributor

Thanks, I think I will add this.

We are also thinking of making it a warning for now. With a note to make it an error next timet the version is bumped.

This comment has been minimized.

@burrows

burrows Jan 31, 2018

Member

@alexraginskiy Good idea. There are likely several more error case checks that could be wrapped in an env check.

@BrianMehrman If you can get the env check working, then I'd prefer to have it throw an exception when an invalid option is detected.

This comment has been minimized.

@BrianMehrman

BrianMehrman Jan 31, 2018

Contributor

I can add the environment check and throw an exception if we are fine with this change breaking things for non production environments.

This comment has been minimized.

@BrianMehrman

BrianMehrman Feb 4, 2018

Contributor

Added the check for the production environment here. 6d9339afab6

@burrows

This comment has been minimized.

Copy link
Member

burrows commented Feb 6, 2018

@BrianMehrman After a bit more thought, I'm not sure this is the best time to add the dependency on the process.env. I'm afraid it could cause issues in other build environments. Lets go back to just logging a warning.

@BrianMehrman BrianMehrman force-pushed the add_property_error branch from 6d9339a to 980f270 Feb 15, 2018

@BrianMehrman

This comment has been minimized.

Copy link
Contributor

BrianMehrman commented Feb 15, 2018

I removed the process.env check and added a console.warn instead of throwing an error.

Let me know if anyone else has any other feedback.

@burrows

This comment has been minimized.

Copy link
Member

burrows commented Feb 15, 2018

LGTM. Go ahead and merge when you are ready.

@BrianMehrman BrianMehrman merged commit f686707 into master Mar 2, 2018

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