Skip to content

Conversation

@khankuan
Copy link
Contributor

@khankuan khankuan commented Aug 3, 2015

Closes #354

@bruderstein
Copy link
Collaborator

Hi, it'd be awesome if you could add a test or two for when this prop is false 😄

It'd be worth noting in the README too, that setting completed=true has no effect when allowCache=false

@khankuan
Copy link
Contributor Author

khankuan commented Aug 6, 2015

Done :) Is the test sufficient?

@bruderstein
Copy link
Collaborator

Perfect, thanks!

If you rebase on master, you'll see the createControl() helper can be used now to create the control with the given props, which shortens the boilerplate code in the beforeEach()

This means your beforeEach() becomes simply

instance = createControl({
    value: '',
    asyncOptions: asyncOptions,
    allowCache: false

```js
instance = createControl({
    value: '',
    asyncOptions: asyncOptions,
    allowCache: false
});

(onChange is automatically set by createControl)

@khankuan
Copy link
Contributor Author

khankuan commented Aug 7, 2015

Alrights updated ;)

JedWatson added a commit that referenced this pull request Aug 8, 2015
@JedWatson JedWatson merged commit 1a476cc into JedWatson:master Aug 8, 2015
@JedWatson
Copy link
Owner

Great - thanks for the PR @khankuan and the review @bruderstein!

@JedWatson
Copy link
Owner

As I was writing up the release notes for 0.6.1, I realised that I think it's clearer if we flip this around so there's a disableCache option, rather than having enableCache default to true.

Having an option that needs to be set to turn something off (caching) seems more intuitive if it's written in the negative, rather than having to specify a false value for a positive option.

If the prop should be in the positive by default, we should name the feature (similar to autoload); possibly cacheAsyncResults would be clearer.

I want to release a new version with the PRs that have just been merged, but will leave some time for feedback on this so we don't have to release a breaking change if there's a better option.

I've committed the change to make it disableCache, let me know what you think.

@khankuan
Copy link
Contributor Author

khankuan commented Aug 8, 2015

Ah good point ;) cacheAsyncResults sounds great. Now that you've mentioned, disableCache seems to be ambiguous as well since cache only applies to async results in this case.

@bruderstein
Copy link
Collaborator

I'm generally against toggles named negatively, they remind me of the opt-out boxes on marketing forms ("tick here if you don't want to not receive email from us" 😄), but I see the clarity problem here.

I think I'm with @khankuan on this, and cacheAsyncResults with default as true would be my vote.

@JedWatson
Copy link
Owner

I thought of cacheAsyncResults about a minute after I committed the change to disableCache... 😄

@bruderstein that marketing form connection is pretty funny, I may never use a negative toggle again...

Just committed the change, so we now have cacheAsyncResults.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3842f8e on khankuan:develop/cache-option into ** on JedWatson:master**.

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.

4 participants