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

Caching and caching tests improvements #1808

Merged
merged 6 commits into from Jan 13, 2015

Conversation

Projects
None yet
3 participants
@TimothyGu
Member

TimothyGu commented Jan 3, 2015

No description provided.

TimothyGu added some commits Jan 2, 2015

Add rudimentary tests for compileClient()
It is already covered by some other tests, but not directly, resulting
in an uncovered branch.

Places already covering this function:

1. Deprecation test for `options.client` (under-the-hood)
2. All the test cases
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 11, 2015

Member

I'll merge this in two days if no one reviews, to keep PRs from rotting.

Member

TimothyGu commented Jan 11, 2015

I'll merge this in two days if no one reviews, to keep PRs from rotting.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jan 12, 2015

Member

If we did,handleSrcCache as:

function handleFileCache (options) {
  if (options.cache && exports.cache[options.filename]) {
    return exports.cache[options.filename];
  } else {
    var result = exports.compile(fs.readFileSync(options.filename, 'utf8'), options);
    if (options.cache) exports.cache[options.filename] = result;
    return result;
  }
}

We could skip the extra (totally pointless) cache of the source strings.

Member

ForbesLindesay commented Jan 12, 2015

If we did,handleSrcCache as:

function handleFileCache (options) {
  if (options.cache && exports.cache[options.filename]) {
    return exports.cache[options.filename];
  } else {
    var result = exports.compile(fs.readFileSync(options.filename, 'utf8'), options);
    if (options.cache) exports.cache[options.filename] = result;
    return result;
  }
}

We could skip the extra (totally pointless) cache of the source strings.

@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel Jan 12, 2015

Contributor

(2nd if's condition should read options.cache)

Contributor

vendethiel commented Jan 12, 2015

(2nd if's condition should read options.cache)

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jan 12, 2015

Member

Thanks, updated

Member

ForbesLindesay commented Jan 12, 2015

Thanks, updated

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 12, 2015

Member

IIRC I tried that but it didn't work for some obscure reason I do not recall right now. Will get back to you.

Member

TimothyGu commented Jan 12, 2015

IIRC I tried that but it didn't work for some obscure reason I do not recall right now. Will get back to you.

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 12, 2015

Member

Updated PR. Removed handleSrcCache() altogether.

Member

TimothyGu commented Jan 12, 2015

Updated PR. Removed handleSrcCache() altogether.

Show outdated Hide outdated lib/index.js

ForbesLindesay added a commit that referenced this pull request Jan 13, 2015

Merge pull request #1808 from jadejs/caching-test
Caching and caching tests improvements

@ForbesLindesay ForbesLindesay merged commit d63928a into master Jan 13, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ForbesLindesay ForbesLindesay deleted the caching-test branch Jan 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment