Transpilation Optimization #115
Conversation
HaidarJbeily7
force-pushed
the
37/transpile-check-sources
branch
from
b965afc
to
c29cfee
Compare
|
@maxonfjvipon I have made the requested changes. |
|
Closes: #37 |
| @@ -65,9 +73,28 @@ describe('transpile', function() { | |||
| }); | |||
| ['simple-test', 'alone-test'].forEach((name) => { | |||
| it(`should generate test JS file for ${name}`, function() { | |||
| assertFilesExist(transpile(name), target, [`project/com/eo2js/${name}.test.js`]) | |||
| prepare(name) | |||
There was a problem hiding this comment.
@HaidarJbeily7 it became a little bit messy because prepare is called twice in beforeEach and in the test. I think the design of the helper functions here should be changed somehow
There was a problem hiding this comment.
@maxonfjvipon I enhanced the helper functions a little bit. Please check again!
| */ | ||
| const transpile = function(name = 'simple') { | ||
| prepare(name) | ||
| return runSync([ |
There was a problem hiding this comment.
@HaidarJbeily7 I think you can call retranspile here to reduce code duplication
There was a problem hiding this comment.
I think calling retranspile in transpile may result in a code smell because of two reasons:
- As an abstraction, re-something should call something not vice-versa
- What if we need to add more logic regarding retranspilation here in the future? This would cause a problem as those two functions are tightly coupled.
Because of the above, I decided to duplicate the code. What do you think?
One solution I thought of to avoid code duplication:
- Remove
retranspileand Modify the transpile function to accept the prepare boolean flag here. So, when we re-transpile we don't resume to the initial state (copying files on Windows gives a carbon copy of the source file. I mean the same mtime -> resulting test failing)
| const transpiled = path.resolve(target, '8-transpile/com/eo2js/simple.xmir') | ||
| const source = path.resolve(target, '6-verify/com/eo2js/simple.xmir') | ||
| const first = fs.statSync(transpiled).mtime | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)) |
There was a problem hiding this comment.
@HaidarJbeily7 why do we need timeout here?
There was a problem hiding this comment.
@maxonfjvipon Testing on windows might fail without deterministic waiting time here as for a case when a file is updated in a small timespan, it may result test failing here. (first and second mtime are equal)
|
@rultor merge |
@maxonfjvipon OK, I'll try to merge now. You can check the progress of the merge here. |
@maxonfjvipon Done! FYI, the full log is here (took me 20min). |
|
@HaidarJbeily7 Thanks |
Overview:
Added utility function to skip unnecessary transpilation when source files haven't been modified. This optimization compares file modification timestamps to determine if retranspilation is needed, improving build performance.
Changes:
PR-Codex overview
This PR refactors the transpilation process by introducing a
preparefunction, modifying thetranspilefunction to use it, and adding aneedsRetranspilecheck to optimize file processing. It also enhances test cases for retranspilation based on file modifications.Detailed summary
transpilefunction toprepare.transpilefunction that callsprepare.retranspilefunction to handle cases when files are modified.needsRetranspilefunction to check modification times.transformfunction to useneedsRetranspile.