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

Worker: Babel ignore node_modules by default + some minor babel transform perf improvements #435

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JHilker
Copy link
Contributor

@JHilker JHilker commented May 27, 2021

I ended up digging a bit into the performance here since I noticed that with newer versions of jscodeshift, what used to be quick (under Jests 5s timeout) was now consistently taking over 10 seconds.

(Aside, it actually was taking closer to 30s when I first started, but I believe this was due to an insanely large default BABEL_CACHE_PATH that consistently took over 5s to read from disk. Deleting that was my biggest perf win by far)

For the purposes of my testing, I was running a jest test that also happened to run jscodeshift via the node API. This task also imported several larger dependencies.

After the cache was cleared, via performance timing in the setup function, this function took an average of 14.157 seconds across 5 runs (13.661, 13.479, 13.438, 15.136, 15.071) with a hot cache and 11.408 seconds across 5 runs (12.133, 11.419, 11.321, 11.201, 10.965) with the cache disabled with the BABEL_DISABLE_CACHE env variable.

I'll probably continue to use BABEL_DISABLE_CACHE in my tooling going forward due to it seemingly being a performance improvement when babel/register is used like this, so all numbers going forward will make the assumption that it is being used.

The swap to require plugins before babel/register installed it hooks and reverting those hooks after the transform were required aren't too significant, but seemed worth including since I already had tested them locally. They reduced the average time to 10.571 seconds across 5 runs (10.769, 10.884, 10.379, 10.510, 10.312), a 7.3% decrease. Both these changes relate to just avoiding having require logic go through babel/register if it doesn't need to, either be requiring it before it hooks in, or removing the hooks before further requires occur.


The bigger, and more controversial change, is adding /node_modules/ to the ignore list. This was proposed in #294 (comment), but I'm not sure if there is more context there. By default babel/register would ignore the node_modules of the cwd, but we explicitly want to compile files outside of the cwd here 🤔 . Should this be scoped to the transforms directory? Any node_modules in parent directories of the transform? All node_modules? Should it be configurable instead?

For my transform specifically, this reduced the setup time down to an average of 2.536 seconds across 5 runs (2.866, 3.169, 2.553, 1.632, 2.458), and additional 76% decrease from before. Though this is obviously dependent what dependencies a transform is pulling in

I'm happy to split the /node_modules/ change into a separate PR for further discussion if desired, but figured I'd put it all here for now since they are a bit related.

src/Worker.js Outdated

if (babelRegister) {
babelRegister.revert();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test:

it('transpiles imported Typescript files in transform files', () => {
const source = createTempFileWith('a');
const helper = createTempFileWith(
'module.exports = function(x: string): x is string {};',
undefined,
'.ts'
);
const transform = createTransformWith(
`return require('${helper}').toString();`,
'.ts'
);
return Promise.all([
run(['-t', transform, source]).then(
args => {
expect(readFile(source).toString())
.toMatch(/function\s*\(x\)\s*{}/);
}
),
]);
});

Catches that while reverting the babel register here works for any require/import that would be run at require time, it wouldn't catch any that would only be executed at run time.

This part isn't too significant, so supporting that case probably more important than worrying about reverting the register? Happy to 🔪

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

Successfully merging this pull request may close these issues.

2 participants