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

Can we remove the max() from staleIn? #166

Closed
EyePulp opened this issue Jul 9, 2016 · 5 comments
Closed

Can we remove the max() from staleIn? #166

EyePulp opened this issue Jul 9, 2016 · 5 comments
Assignees
Labels
bug
Milestone

Comments

@EyePulp
Copy link
Contributor

@EyePulp EyePulp commented Jul 9, 2016

Looking here:
https://github.com/hapijs/catbox/blob/master/lib/policy.js#L282

staleIn has a max possible value of 1 ms less than a single day:
staleIn: [Joi.number().integer().min(1).max(86400000 - 1), Joi.func()],

Whereas expiresIn simply has a minimum:
expiresIn: Joi.number().integer().min(1),

Can we remove the max on staleIn or is it there for a reason? I can make a PR if that's helpful, and I don't see any tests that are explicitly enforcing the behavior.

@legraphista

This comment has been minimized.

Copy link

@legraphista legraphista commented Mar 10, 2017

+1
I can see the staleIn having to be lower than expiresIn, but I don't really understand the reason behind limiting it to 1 day

@EyePulp

This comment has been minimized.

Copy link
Contributor Author

@EyePulp EyePulp commented Mar 10, 2017

@legraphista One workaround to staleIn supports providing a function rather than a fixed integer, at which point you can return any value :

//a convenience wrapper we use to create cache settings
// ms() is an npm module for defining readable millisecond counts
const cacheDefaults = function (settings,genKeyFn,expiresIn=ms('7 days'),staleIn=ms('1 day')){

    expiresIn  = settings.isdev?ms('2 min'):expiresIn;
    staleIn    = settings.isdev?ms('90 sec'):staleIn;

    const staleInFn=()=>staleIn;

    return {
        bind        : {settings:settings},
        generateKey : genKeyFn,
        cache       : {
            cache           : 'redisCache',
            generateTimeout : ms('60 sec'),
            expiresIn       : expiresIn,
            staleIn         : staleInFn,
            staleTimeout    : 100,
        },
    };
};
@hueniverse hueniverse added the feature label Mar 21, 2017
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Mar 21, 2017

It's been like this since ever. Not sure why. I will review it one more time to make sure I am not forgetting something but doesn't look like we need this limit anymore.

@hueniverse hueniverse added bug and removed feature labels Mar 21, 2017
@EyePulp

This comment has been minimized.

Copy link
Contributor Author

@EyePulp EyePulp commented Mar 21, 2017

Thanks @hueniverse =)

@hueniverse hueniverse self-assigned this May 27, 2017
@hueniverse hueniverse added this to the 7.1.4 milestone May 27, 2017
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented May 27, 2017

I knew there had to be a reason for that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.