isGetter doesn't actually validate getters

[appsforartists]

I'd expect isGetter to assert that a getter is valid (e.g. composed only of strings, optionally a trailing function, or other getters). Instead, it tests that an array ends in a function.

Valid getters that fail this test

["path"]

Invalid getters that pass

[1, true, null, () => ({})]

A more robust test

function isGetter(value) {
  return isArray(value) && value.every(
    (item, i) =>    isString(item)
                 || (i === value.length - 1 && isFunction(item))
                 || isGetter(item)
  );
}

It passes all the current getter tests.

[colindresj]

👍 on this. Early on I was doing some pseudo-type checking on getters using isGetter, but decided to drop that approach because I didn't find it reliable enough.

[jordangarcia]

Sure I can implement this to be a true isGetter check.

I was hesitant to do it at first since its a potentially expensive operation that will happen recursively.

@colindresj if you are planning to add these checks to your code I would wrap them in a flag that removes them in production code

[appsforartists]

I can open a PR if you like. I hesitated because I didn't know if changing isGetter to be more complicated would be discouraged for perf reasons.

[colindresj]

Definitely @jordangarcia. I think this kind of approach is useful in dev, where you don't care about perf.

In the future, I actually think Nuclear should automatically turn this (and other dev-only helpers) off when in production, kind of like React does with PropTypes, et al. That means updating how Nuclear config is currently handled though, so definitely a different issue entirely, and for now devs should take care of this themselves.

[jordangarcia]

@colindresj yeah its something i've been thinking about

as to having a nuclear.js or nuclear-debug.js be the version with the __DEV__ flag on and also having invariant checks like isGetter.

I like the idea of keeping the dispatch logs in the code as something that can be configured when you instantiate a Reactor. At optimizely we have a flag that turns debugging on in production which has been immensely helpful when debugging issues that have already made it to prod.

I want to think about this a bit more before we decide anything. I am currently leaning towards adding more invariant checks and then having the minified version remove them.

Also: the way npm's package.json main works is to point at the unminified version of Nuclear. I'd like to be able to allow people to use npm but get a version of Nuclear without the invariants. Anyone have ideas?

[appsforartists]

@jordangarcia I think React has one version for npm, but has a const (NODE_ENV === "production") that determines whether or not the tests run. They presume that if you care about perf, you'll run it through Webpack and Uglify, which will strip the unreachable blocks.

[colindresj]

In addition, React has a set of npm-specific grunt tasks for building the unminified main file with the conditional checks that @appsforartists mentioned, as well as a dist folder which includes minified files for React, addons, etc. I really like the approach, because you can build the library agnostic of where it'll be hosted, then have specific tasks for publishing it to NPM, etc.

The directory that eventually hits NPM is here: https://github.com/facebook/react/tree/master/npm-react

And the tasks for generating the files that get put in the directory are here: https://github.com/facebook/react/blob/master/grunt/tasks/npm-react.js

There's a release task that eventually publishes React everywhere it needs to: https://github.com/facebook/react/blob/master/grunt/tasks/release.js

[jordangarcia]

I am a bit uncomfortable adding this stricter validation without having a good way to remove it, and that will require rethinking the __DEV__ approach.

Punting this to 1.1.x