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

add maxAge option for alfy.cache - fixes #3 #4

Merged
merged 2 commits into from
Jul 13, 2016
Merged

add maxAge option for alfy.cache - fixes #3 #4

merged 2 commits into from
Jul 13, 2016

Conversation

SamVerschueren
Copy link
Collaborator

  • Extended the Conf class and override the set, get and has methods
  • Added tests for the cache
  • Added documentation regarding the maxAge option

As described in #3, not sure if we should do it with the CacheConf object here, add that option to Conf or create an external CacheConf module to handle this. Will to do either one of those :).

Feel free to suggest improvements of the code or docs.

@SamVerschueren
Copy link
Collaborator Author

SamVerschueren commented Jul 11, 2016

The downside of this approach is that it isn't possible to implement a fallback mechanism like I do in alfred-ng2. If the cache is expired, it tries to load the remote data, if that fails it will still load the "old" cache. It would be nice if it was possible to still support that.


It actually is possible if you use alfy.cache.store directly, but I don't want everyone to skip the methods. Maybe we should not remove the item if the data is expired and add an extra option to the get method in order to force retrieval.

alfy.cache.set('foo', 'bar', {maxAge: 5000});

await delay(5000);

alfy.cache.get('foo');
//=> undefined

alfy.cache.get('foo', true);
//=> 'bar'

@sindresorhus
Copy link
Owner

The downside of this approach is that it isn't possible to implement a fallback mechanism like I do in alfred-ng2. If the cache is expired, it tries to load the remote data, if that fails it will still load the "old" cache. It would be nice if it was possible to still support that.

Hmm, maybe we should bundle got here too, and give it ability to cache requests? That would solve your use-case.

@sindresorhus sindresorhus changed the title add maxAge option for alfy.cache - resolves #3 add maxAge option for alfy.cache - fixes #3 Jul 12, 2016
@SamVerschueren
Copy link
Collaborator Author

Not sure if we should bundle got into alphy. How would the interface look like then?

alfy.got.get('https://myapi.com/foo/bar.json', {maxAge: 5000}).then(res => {
    console.log(res.body);
    // {foo: 'bar'}
});

@sindresorhus
Copy link
Owner

@SamVerschueren Fetching from network is a very common operation in Alfred workflows. Would be nice to simplify it. Dependencies are super cheap anyways as they're bundled with the workflow, not npm install'd.

I'm thinking:

alfy.fetch('https://myapi.com/foo/bar.json', {maxAge: 5000}).then(res => {
    console.log(res.body);
    // {foo: 'bar'}
});

And maybe even make {json: true} the default.

To be clear, this is in addition what's currently in this PR, not instead of. Should probably be a separate PR.

@SamVerschueren
Copy link
Collaborator Author

SamVerschueren commented Jul 12, 2016

So we would only expose the get method right? Don't think that post makes sense in an Alfred app. Except if the API endpoint expects a post request with alfy.input as post data. Nah, let's just expose the get as fetch. If they have to use post, they can always install got and use got directly instead of via alfy.

Dependencies are super cheap anyways as they're bundled with the workflow

👍

Should probably be a separate PR.

Will add it after this one gets merged.

Is it ok to remove the cached data then when it is expired? Or should we just ignore it without removing it?

@sindresorhus
Copy link
Owner

So we would only expose the get method right?

Yup.

Except if the API endpoint expects a post request with alfy.input as post data.

Then they can just pass the method option. We just pass the options object to got.

Is it ok to remove the cached data then when it is expired? Or should we just ignore it without removing it?

For alfy.cache, remove it. For alfy.fetch, remove it after successfully getting new data.

@SamVerschueren
Copy link
Collaborator Author

Then they can just pass the method option. We just pass the options object to got.

👍

For alfy.cache, remove it. For alfy.fetch, remove it after successfully getting new data.

👍


```js
alfy.cache.set('foo', 'bar', {maxAge: 5000});

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do a alfy.cache.get('foo'); and show the value before the delay too. For clarity.

Example:

```js
const delay = require('delay');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer a require at the top instead of await require('delay')(5000). But if you want it inline, I'll fix it :).

@sindresorhus sindresorhus merged commit 1272827 into master Jul 13, 2016
@sindresorhus sindresorhus deleted the iss3 branch July 13, 2016 13:11
@sindresorhus
Copy link
Owner

Looks superb, as always :)

@SamVerschueren
Copy link
Collaborator Author

Awesome. Super nice to see this land :)! If #6 is implemented, I nearly don't have to do anything anymore in my alfred-ng2 plugin 😆 .

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.

2 participants