Add caching #77

joeyparrish · 2023-08-27T01:01:07Z

Caches processed sources with an md5 hash of the original.

An updated and slightly-simplified version of #34

Closes #28

Caches processed sources with an md5 hash of the original. An updated and slightly-simplified version of babel#34

joeyparrish · 2023-08-27T02:47:18Z

Compared to the original change in #34, this makes the cache path configurable, with the default being not to cache at all. It also avoids re-reading the file, since we are provided with the contents.

shuhei · 2023-08-27T13:14:57Z

lib/index.js

@@ -2,6 +2,9 @@

 var babel = require('@babel/core');

+var LocalCache = require('node-localcache');


I'm reluctant to add this package as a dependency. It has more dependencies with fixed versions and hasn't been maintained for 7 years.

Can caching be implemented without additional dependencies or a more popular and maintained package?

Seconded.

The implementation of node-localcache writes all cache entries as one big JSON object to a single file. This sounds bad for a preprocessor: if you're running Karma in watch mode, a small change to a single file inside a large project means (repeatedly) overwriting a massive JSON file. 😕

I would prefer an alternative caching solution that is more gentle with the file system.

I inherited that from the original PR (#34). Writing files without deps is not hard. And writing to individual files instead of one big one is also easy. I'll revise it.

Do you require old-style callbacks in this project or can I use fs/promises?

Babel 7 supports Node 6 and up, which unfortunately means that both fs.promises and util.promisify() are off the table... 😞 (Or we'd have to wait for Babel 8...)

shuhei · 2023-08-27T13:24:01Z

lib/index.js

@@ -10,20 +13,38 @@ function createPreprocessor(args, config, logger, helper) {
  var log = logger.create('preprocessor.babel');
  config = config || {};

-  function preprocess(content, file, done) {
-    log.debug('Processing "%s".', file.originalPath);
+  var cachePath = config.cachePath;


Could you make this work with custom processors as well? var cachePath = args.cachePath || config.cachePath;

Yes! I'll do that. I wasn't familiar with that detail.

shuhei

Thanks for the PR! I added a few comments. Especially, the dependency one needs to be addressed before merging this PR.

Please note that Karma test runner itself has been deprecated. https://github.com/karma-runner/karma. This repo is going to be deprecated as well. So, further investment into this repo might not be worth your time.

shuhei · 2023-08-27T13:27:52Z

lib/index.js

  delete customConfig.base;

+  // Ignore 'cachePath', which we use here instead of passing to babel.
+  delete baseConfig.cachePath;


Please don't mutate objects that this function doesn't create. Could you copy it and remove the property like it was/is done for base? Also, could you also do the same for customConfig?

Maybe you could define a helper function to copy a given object and remove given properties.

shuhei · 2023-08-27T13:34:49Z

lib/index.js

+        done(null, code);
+      } else {
+        log.debug('Processing "%s" - Cache hit.', file.originalPath);
+        done(null, cacheEntry.code);


This function got a bit convoluted now. Also, the cache miss/hit messages are logged even when cache is disabled. Could you extract the caching logic to a function? For example:

var transformFunc = cache ? transformWithCache : transform; var code = transformFunc(content, file); function transform(content, file) { var babelOptions = helper.merge({}, options, { filename: file.originalPath }); var processed = babel.transform(content, babelOptions); return processed ? processed.code : content; } function transformWithCache(content, file) { // Uses caching and the transform function. }

This caches Babel's transpiler output for reuse, and should speed up all test runs in theory, but the effect is most noticeable on local test runs. This uses a fork of karma-babel-preprocessor, which contains babel/karma-babel-preprocessor#77. If/when that PR is merged, we can move back to the upstream module. Local runs will start faster because only modified source files will be re-processed through Babel when the tests start up. In the Selenium workflow, Babel output and node_modules will both be computed by the singular build-shaka job, stored, and then reused by all the Selenium lab matrix jobs. On my workstation (3.3 GHz cores, 32GB RAM, spinning platter disk), I see tests start about ~60 seconds faster. In the lab (2.1-4.7 GHz cores, 64GB RAM, solid-state disk), I see tests start about ~10 seconds faster.

joeyparrish · 2023-08-28T16:35:02Z

Please note that Karma test runner itself has been deprecated. https://github.com/karma-runner/karma. This repo is going to be deprecated as well. So, further investment into this repo might not be worth your time.

Ah, classic Google.

This caches Babel's transpiler output for reuse, and should speed up all test runs in theory, but the effect is most noticeable on local test runs. This uses a fork of karma-babel-preprocessor, which contains babel/karma-babel-preprocessor#77. If/when that PR is merged, we can move back to the upstream module. Local runs will start faster because only modified source files will be re-processed through Babel when the tests start up. In the Selenium workflow, Babel output and node_modules will both be computed by the singular build-shaka job, stored, and then reused by all the Selenium lab matrix jobs. On my workstation (3.3 GHz cores, 32GB RAM, spinning platter disk), I see tests start about ~60 seconds faster. In the lab (2.1-4.7 GHz cores, 64GB RAM, solid-state disk), I see tests start about ~10 seconds faster.

joeyparrish · 2024-03-11T16:56:14Z

Closing this PR as we may abandon Karma.

Stephen Bussey and others added 5 commits August 27, 2023 00:42

Add caching

9e7f4c2

Caches processed sources with an md5 hash of the original. An updated and slightly-simplified version of babel#34

Fix dependency config

05336fa

Add .cache to .gitignore

c12c3e0

Make the cache path configurable

02fd7b6

Remove .cache from .gitignore, now that the path is configurable

2fb7d36

Remove fs

5aff3bb

This was referenced Aug 27, 2023

Adding in caching #28

Open

test: Cache Babel output shaka-project/shaka-player#5539

Merged

shuhei reviewed Aug 27, 2023

View reviewed changes

shuhei requested changes Aug 27, 2023

View reviewed changes

joeyparrish closed this Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add caching #77

Add caching #77

joeyparrish commented Aug 27, 2023 •

edited

Loading

joeyparrish commented Aug 27, 2023

shuhei Aug 27, 2023

MattiasBuelens Aug 27, 2023

joeyparrish Aug 28, 2023

MattiasBuelens Aug 28, 2023

shuhei Aug 27, 2023

joeyparrish Aug 28, 2023

shuhei left a comment

shuhei Aug 27, 2023

joeyparrish Aug 28, 2023

shuhei Aug 27, 2023

joeyparrish Aug 28, 2023

joeyparrish commented Aug 28, 2023

joeyparrish commented Mar 11, 2024

		@@ -2,6 +2,9 @@

		var babel = require('@babel/core');

		var LocalCache = require('node-localcache');

Add caching #77

Add caching #77

Conversation

joeyparrish commented Aug 27, 2023 • edited Loading

joeyparrish commented Aug 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuhei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyparrish commented Aug 28, 2023

joeyparrish commented Mar 11, 2024

joeyparrish commented Aug 27, 2023 •

edited

Loading