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

feat: support a few domains (500) #283

Merged

Conversation

nightnei
Copy link
Contributor

No description provided.

@nightnei nightnei requested a review from StyleT April 16, 2021 10:00
@nightnei nightnei changed the title feat: support a few domains (500 template) feat: support a few domains (500) Apr 16, 2021
@nightnei nightnei force-pushed the feature/supportFewDomains_500 branch from ed612be to 66b0652 Compare April 16, 2021 11:30
Copy link
Contributor

@StyleT StyleT left a comment

Choose a reason for hiding this comment

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

You have failed tests :(

@StyleT
Copy link
Contributor

StyleT commented Apr 16, 2021

Pls write E2E tests for this

Base automatically changed from feature/supportFewDomains_UI to feature/supportFewDomains_master April 16, 2021 15:05
@nightnei nightnei force-pushed the feature/supportFewDomains_500 branch from 8fcaf18 to b2ce5be Compare April 19, 2021 11:50
@nightnei
Copy link
Contributor Author

You have failed tests :(

fixed

@nightnei
Copy link
Contributor Author

Pls write E2E tests for this

done

@nightnei nightnei requested a review from StyleT April 19, 2021 12:30
@@ -6,13 +6,14 @@ errors.WrapWithCacheError = extendError('WrapWithCacheError');
const wrapWithCache = (localStorage, logger, createHash = hashFn) => (fn, cacheParams = {}) => {
const {
cacheForSeconds = 60,
name = '', // "hash" of returned value is based only on arguments, so with the help "name" we can add prefix to hash
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a required parameter.

} = cacheParams;

const cacheResolutionPromise = {};

return (...args) => {
const now = Math.floor(Date.now() / 1000);
const hash = args.length > 0 ? createHash(JSON.stringify(args)) : '__null__';
const hash = `${name ? name + '__' : ''}${args.length > 0 ? createHash(JSON.stringify(args)) : '__null__'}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass name to hash function. Don't do a concatenation after

Copy link
Contributor

Choose a reason for hiding this comment

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

So now we would use hash fn even if we don't have args

@nightnei nightnei merged commit 342a2dc into feature/supportFewDomains_master Apr 20, 2021
@nightnei nightnei deleted the feature/supportFewDomains_500 branch April 20, 2021 07:39
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