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

How to dynamically mark ESM loader cache as dirty with Istanbuljs? #595

Closed
deepsweet opened this issue Sep 9, 2018 · 11 comments
Closed
Labels

Comments

@deepsweet
Copy link

deepsweet commented Sep 9, 2018

Here is some not very helpul background on the issue from the past (clearing Babel cache manually doesn't help because it feels like ESM loader writes its cache on exit).

Start runner has a CLI entry point, which applies both ESM loader and babel/register, and then loads tasks from tasks file on the fly using that transpilation combo.

Start runner has its own Istanbuljs plugin, which instruments sources, runs tests and then collects and prints out coverage results. Kinda Nyc, but without the Nyc part, i.e. manually.

There is a task called "test" which contains Istanbuljs plugin. If I run Start own tests using this command:

yarn start test

Then everything is fine. Start loads ESM + babel/register, loads tasks file, loads Istanbuljs plugin, invokes "test" task, applies Istanbuljs instrumentation import-hook on Start own source files, runs tests that imports those sources so Istanbuljs applies instrumentation by the hook, collects coverage and reports it.

Then if I change some of the involved into test task source files, for example add some new variable to packages/plugin-lib-istanbul/src/instrument.ts and run yarn start test again I get errors (sometimes it takes few runs) about instrumented by Istanbul code stored in Babel cache which is somehow wrong from ESM loader perspective:

ReferenceError: cov_1yb4aus0im is not defined
    at default (@start/packages/plugin-find/src/index.ts:1:1)

The only thing that helps:

rm -rf node_modules/ && yarn
# rm -rf node_modules/.cache does NOT help, only entire `node_modules/` (btw, why?)

And then to always use it like this:

env NYC_ROOT_ID=1 yarn start test

i.e. to pretend that it's Nyc to mark ESM cache as dirty.

But if I want to hide this from CLI part to internals of test task, or better of Istanbljs plugin, then it doesn't work – at that time ESM loader has been already loaded and isNyc is false regardless any later changes of NYC_ROOT_ID env var.

Is it possible to check for that env var explicitly every time onExit is called, not from ../env/is-nyc.js? From what I see it should help.

Also, since it's not really Nyc, could you please add another more abstract env var? ESM_DISABLE_CACHE or something so that Start doesn't have to mimic.

I've been struggling with this issue for many months since I started using ESM loader in my projects, but never had time to finally try to explain it, it all was very complicated and phantom even for me as the author of Start. Please end my suffering, @jdalton I know that you can handle even such a weird issues.

@jdalton
Copy link
Member

jdalton commented Sep 9, 2018

Hi @deepsweet!

Yep, we can add another environment variable. You can also mark our cache as dirty in a more invasive way by creating a .dirty file in the .cache/esm folder.

Have you checked to see if there's a way to detect standalone istanbul?

@deepsweet
Copy link
Author

deepsweet commented Sep 9, 2018

The trick is not only to add another env var, but to check for it directly, not from the initially imported file, so that it will work if I set the var in between init and close.

I'll try the .dirty thing (probably it will work, but it's too dirty 😐), and I can't find any produced process.env side effects in Istanbul codebase.

Thanks!

@deepsweet
Copy link
Author

$ ls -la node_modules/.cache/esm/.dirty
-rw-r--r-- 1 deepsweet staff 0 Sep  9 22:19 node_modules/.cache/esm/.dirty

$ yarn start test
yarn run v1.9.4
$ node packages/cli/src/index.js test
file:///.../@start/packages/plugin-xargs/src/index.ts:1
ReferenceError: cov_1c3glvsx6u is not defined

or, after another rm -rf node_modules && yarn && yarn start test and changes in some file:

$ yarn start test
yarn run v1.9.4
$ node packages/cli/src/index.js test
ReferenceError: cov_2nv9szp2xf is not defined
    at default (/.../@start/packages/plugin-lib-istanbul/src/instrument.ts:1:1)

So errors content and files are even different from time to time.

@deepsweet
Copy link
Author

Never happens with env NYC_ROOT_ID=1 yarn start test, checked again 3 times in a row.

I found another place in ESM loader affected by isNyc, isSideloaded is true – maybe this is what I want and not onExit with dirty things?

@jdalton
Copy link
Member

jdalton commented Sep 9, 2018

Are you side loading esm (e.g coveragetool -r esm ...)?

@deepsweet
Copy link
Author

The way I load it in Start CLI entry point:

require = require('esm')(module, {
  mainFields: ['module', 'main']
})

@deepsweet
Copy link
Author

deepsweet commented Sep 10, 2018

I did few tests and it seems to work with that latest change in your commit 🎉 Will test it more heavily ASAP.

@deepsweet
Copy link
Author

I can't reproduce it anymore after 15+ runs, changes and reinstalling modules back and forth. I believe that the issue has been fixed.

@jdalton
Copy link
Member

jdalton commented Sep 10, 2018

Rock! I'm going to let this change marinate for a bit before publishing (just to make sure naming is solid).

Update:

OK. Here is my thinking on this. I want to be careful with one-off environment variables like ESM_DISABLE_CACHE. I was wondering if there could be a more generic ESM_OVERRIDES option, but I couldn't think of any other overrides I'd want to apply to ALL packages. The cache option is unique in that regard. I also thought of allowing more than toggling the cache off, like allowing the path to be specified, but I don't like the idea of a global override for that. Toggling it off seems like the safest global override so I'm going to keep it as a one-off for now.

@deepsweet
Copy link
Author

I think it will become clear when you have more issues. ESM_OVERRIDES sounds good for me, as an abstract feature, but its particular use cases are a bit foggy.

@jdalton
Copy link
Member

jdalton commented Sep 17, 2018

v3.0.83 is released 🎉

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

No branches or pull requests

2 participants