Skip to content

Use file hash to check for file changes before transpiling (#299) #313

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

Merged
merged 5 commits into from
Sep 4, 2016
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions src/React.Core/Babel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -350,12 +350,36 @@ string filename
)
{
var outputPath = GetOutputPath(filename);
var sourceMapPath = GetSourceMapOutputPath(filename);
var contents = _fileSystem.ReadAsString(filename);
var result = TransformWithHeader(filename, contents, null);
_fileSystem.WriteAsString(outputPath, result.Code);
_fileSystem.WriteAsString(sourceMapPath, result.SourceMap == null ? string.Empty : result.SourceMap.ToJson());
return outputPath;
if (MustTranspileFile(contents, outputPath))
Copy link
Member

Choose a reason for hiding this comment

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

You could return early here, rather than wrapping the entire thing in the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't do that in the first place is that NOTs (!) in C# is rather subtle and easy to miss. Renaming the MustTranspileFile to e.g. CacheIsValid would allow me to do that. Good suggestion.

It would give two return outputPath; statements, though.

{
var result = TransformWithHeader(filename, contents, null);

var sourceMapPath = GetSourceMapOutputPath(filename);
_fileSystem.WriteAsString(outputPath, result.Code);
_fileSystem.WriteAsString(sourceMapPath, result.SourceMap == null ? string.Empty : result.SourceMap.ToJson());
}
return outputPath;
}

/// <summary>
/// Checks whether an input file (given as inputFileContents) should be transpiled
/// by calculating the hash and comparing it to the hash value stored
/// in the file given by outputPath. If the outputPath file does not
/// exist the input file should always be transpiled.
/// </summary>
/// <param name="inputFileContents">The contents of the input file.</param>
/// <param name="outputPath">The output path of the (possibly previously) generated file.</param>
/// <returns>Returns true if the file should be transpiled, false otherwise.</returns>
public virtual bool MustTranspileFile(string inputFileContents, string outputPath)
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer calling this ValidateCache and making it return true if the cache is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ValidateCache is rather vague and unspecific - CacheIsValid would be a better name, since it better hints at what state the cache is actually in, when true/false is returned.

And I like flipping the true/false values, since it would enable me to turn around the if in your next comment ;)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, CacheIsValid or CheckCacheValidity would be fine.

Also please make it private, and it does not need to be virtual.

{
if (!File.Exists(outputPath))
return true;

var hashForInputFile = _fileCacheHash.CalculateHash(inputFileContents);
var existingOutputContents = _fileSystem.ReadAsString(outputPath);
var fileHasNotChanged = _fileCacheHash.ValidateHash(existingOutputContents, hashForInputFile);
return !fileHasNotChanged;
}
}
}