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

Handle invalidating cache if dependency is a globAsset #1417

Merged
merged 5 commits into from
Jul 7, 2018

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented May 22, 2018

Duplicate of #1414

This is needed to properly invalidate and cache styl files as they allow glob requires.

But I kind of messed up a rebase, so here is a clean version

Closes #1546

@DeMoorJasper DeMoorJasper changed the title Handle invalidating cache if dependency is a glob Handle invalidating cache if dependency is a globAsset May 22, 2018
src/FSCache.js Outdated
mtime = fileStats.mtime.getTime();
}
}
} else {
Copy link
Contributor

@fathyb fathyb May 23, 2018

Choose a reason for hiding this comment

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

nit-pick: use Promise.all for better perf:

if (isGlob(filename)) {
    let files = await glob(filename, {
        strict: true,
        nodir: true
    });

    mtime = (
        await Promise.all(
            files.map(file =>
                fs.stat(file).then(({mtime}) => mtime.getTime())
            )
        )
    ).reduce((a, b) => Math.max(a, b), 0)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change it

@devongovett
Copy link
Member

Maybe it would simplify things if we just marked all GlobAssets as non-cacheable? That way the cache wouldn't need to have specific logic about them. Alternatively we could use the shouldInvalidate hook in GlobAsset to check that...

Right now we always need to run glob for GlobAssets every time, and that's the expensive part. So if that's the case, we might as well just not cache them at all. WDYT?

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented May 23, 2018

@devongovett Won't this invalidate the parent asset as well? as the main edit in this PR is dependencies.
Not sure if this would still be faster if the parent asset is a fairly large js file in production mode.

@devongovett
Copy link
Member

When you require('./foo/*.js') for example, a virtual file (the GlobAsset) is created which looks something like this:

module.exports = {
  test: require('./foo/test.js'),
  bar: require('./foo/bar.js')
};

That's the "file" that you are needing to invalidate when files are added/removed. So if we just don't cache that at all, it should just work for free. Not sure we would gain anything by caching it since we need to check it every time anyway.

@codecov-io
Copy link

codecov-io commented May 24, 2018

Codecov Report

Merging #1417 into master will increase coverage by 1.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1417      +/-   ##
=========================================
+ Coverage   87.96%   89.1%   +1.13%     
=========================================
  Files          80      81       +1     
  Lines        4653    4679      +26     
=========================================
+ Hits         4093    4169      +76     
+ Misses        560     510      -50
Impacted Files Coverage Δ
src/FSCache.js 98.41% <100%> (+1.74%) ⬆️
src/Parser.js 100% <100%> (ø) ⬆️
src/utils/glob.js 100% <100%> (ø)
src/assets/GlobAsset.js 97.26% <100%> (-1.43%) ⬇️
src/workerfarm/errorUtils.js 82.35% <0%> (-5.89%) ⬇️
src/worker.js 95% <0%> (-5%) ⬇️
src/visitors/dependencies.js 91.45% <0%> (-4.28%) ⬇️
src/Logger.js 95.78% <0%> (-1.06%) ⬇️
src/assets/CoffeeScriptAsset.js 100% <0%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83dfa3e...55951bd. Read the comment docs.

@devongovett devongovett merged commit ec3aea9 into master Jul 7, 2018
@devongovett devongovett deleted the glob-cache-invalidating branch July 7, 2018 23:53
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