Cache is not cleared when transformer changes

[oscard0m]

What happens and why it is wrong

Without clean: true option, the plugin is catching the configuration even if changes at rollup config level are done (changing the implementation of a transformer implementation for instance)

Until verbosity: 3 was not set, it was not discovered cache could be an issue

Questions

• Is there any historical reason to set clean: false as default? It sounds like the default configuration should be without """magic""" on top right?
• What do you think about adding a warning when cache is used so users notice about it with default verbosity level?
• I did not check how cache is implemented but, is it an expected behaviour to not clean the cache when there are changes on the transformers passed to this plugin or is a bug?

Environment

macOS catalina v10.15.4
node 14.2.0

Versions

• typescript: 3.9.3
• rollup: 2.10.5
• rollup-plugin-typescript2: 0.27.1

rollup.config.js

Not relevant

tsconfig.json

Not relevant

package.json

Not relevant

plugin output with verbosity 3

Not relevant

[ezolenko]

This is a bug, the cache is supposed to catch rollup config changes -- the whole rollup config object is hashed and used as a part of cache key. Could you post your rollup config and what kind of change did you make? (or better yet, make a small repo with reproduction).

Thanks.

[ezolenko]

Re warning about cache use, which output message made you realize cache might be involved? Maybe I need to print it on other verbosity levels...

[oscard0m]

This is a bug, the cache is supposed to catch rollup config changes -- the whole rollup config object is hashed and used as a part of cache key. Could you post your rollup config and what kind of change did you make? (or better yet, make a small repo with reproduction).

Thanks.

Sorry for the delay in the response. Here is a simplification of the problem:

import { rollup } from 'rollup';
import typescript from 'rollup-plugin-typescript2';
import ts from 'typescript';

const nodeRemover = context => {
  return sourceFile => {
    const visitor = (node) => {
      if (ts.isImportDeclaration(node)) {
        return undefined;
      }

      return ts.visitEachChild(node, visitor, context);
    };

    return ts.visitNode(sourceFile, visitor);
  };
};

const transformer = () => ({
	before: [nodeRemover],
	after: []
});

rollup({
	input: './index.ts',
	plugins: [
		typescript({
			transformers: [transformer]
		})
	]
}).then(bundle => {
	bundle.write({
		file: 'bundle.js'
	})
})

index.ts

// @ts-ignore
import { a } from './src/a';

const b = 'b';

console.log(a);
console.log(b);

---

When modifying the transformer for this:

...
if (ts.isImportDeclaration(node) || ts.isVariableStatement(node)) {
...

even the original index.ts content did not change, the output will change due to the changes in the transformer. Seems that the plugin is not aware of transformer changes and only watches for the files to be transpiled for applying cache policies.

---

When adding clean: true option, it solves the issue.

[oscard0m]

Re warning about cache use, which output message made you realize cache might be involved? Maybe I need to print it on other verbosity levels...

cache hit and cache miss

[Artur93gev]

Same issue. I'm using typescript-transformer-jsx-remove-attributes transformer.
Need to apply it on test build, but not for production one. So I think the cache is still existing across multiple runs of bundler(rollup in my case).

Thanks to @dominguezcelada . clean: true option solved the issue for me.

[oscard0m]

Hi @ezolenko I'm interested in following up this issue as a chance to get involved in the project if possible. What do you think could be the approach to be followed here?

Once agreed on that I would like to draft a PR for it :)

[ezolenko]

Sorry for the delay, if you want to look into it, check if rollup config changes are reflected in this.rollupConfig here:

rollup-plugin-typescript2/src/tscache.ts

Lines 119 to 129 in 8ec49c7

	this.cacheDir = `${this.cacheRoot}/${this.cachePrefix}${hash(
	{
	version: this.cacheVersion,
	rootFilenames,
	options: this.options,
	rollupConfig: this.rollupConfig,
	tsVersion: tsModule.version,
	},
	this.hashOptions,
	)}`;
	}

And if they are, if they change output of hash.

object-hash can't handle async functions for example. Maybe transformers are being wrapped in async by rollup. Or object-hash doesn't like something else.

The fix might be in object-hash or in rollup too.

[oscard0m]

Sorry for the delay, if you want to look into it, check if rollup config changes are reflected in this.rollupConfig here:

When printing the output of rollupConfig I see the first level of properties are functions, not objects:

{
  name: 'rpt2',
  options: [Function: options],
  watchChange: [Function: watchChange],
  resolveId: [Function: resolveId],
  load: [Function: load],
  transform: [Function: transform],
  generateBundle: [Function: generateBundle],
  _ongenerate: [Function: _ongenerate],
  _onwrite: [Function: _onwrite]
}

Printing transform (which I guess it ends up using code from transformers option, it's constant as far as I can see, no matter the change/s I apply to my custom transformer function):

Details

        transform(code, id) {
            generateRound = 0; // in watch mode transform call resets generate count (used to avoid printing too many copies of the same error messages)
            if (!filter(id))
                return undefined;
            allImportedFiles.add(normalize(id));
            const contextWrapper = new RollupContext(pluginOptions.verbosity, pluginOptions.abortOnError, this, "rpt2: ");
            const snapshot = servicesHost.setSnapshot(id, code);
            // getting compiled file from cache or from ts
            const result = cache().getCompiled(id, snapshot, () => {
                const output = service.getEmitOutput(id);
                if (output.emitSkipped) {
                    noErrors = false;
                    // always checking on fatal errors, even if options.check is set to false
                    const diagnostics = lodash.concat(cache().getSyntacticDiagnostics(id, snapshot, () => {
                        return service.getSyntacticDiagnostics(id);
                    }), cache().getSemanticDiagnostics(id, snapshot, () => {
                        return service.getSemanticDiagnostics(id);
                    }));
                    printDiagnostics(contextWrapper, diagnostics, parsedConfig.options.pretty === true);
                    // since no output was generated, aborting compilation
                    cache().done();
                    if (lodash.isFunction(this.error))
                        this.error(safe.red(`failed to transpile '${id}'`));
                }
                const references = getAllReferences(id, servicesHost.getScriptSnapshot(id), parsedConfig.options);
                return convertEmitOutput(output, references);
            });
            if (pluginOptions.check) {
                const diagnostics = lodash.concat(cache().getSyntacticDiagnostics(id, snapshot, () => {
                    return service.getSyntacticDiagnostics(id);
                }), cache().getSemanticDiagnostics(id, snapshot, () => {
                    return service.getSemanticDiagnostics(id);
                }));
                if (diagnostics.length > 0)
                    noErrors = false;
                printDiagnostics(contextWrapper, diagnostics, parsedConfig.options.pretty === true);
            }
            if (result) {
                if (result.references)
                    result.references.map(normalize).map(allImportedFiles.add, allImportedFiles);
                if (watchMode && this.addWatchFile && result.references) {
                    if (tsConfigPath)
                        this.addWatchFile(tsConfigPath);
                    result.references.map(this.addWatchFile, this);
                    context.debug(() => `${safe.green("    watching")}: ${result.references.join("\nrpt2:               ")}`);
                }
                if (result.dts) {
                    const key = normalize(id);
                    declarations[key] = { type: result.dts, map: result.dtsmap };
                    context.debug(() => `${safe.blue("generated declarations")} for '${key}'`);
                }
                const transformResult = { code: result.code, map: { mappings: "" } };
                if (result.map) {
                    if (pluginOptions.sourceMapCallback)
                        pluginOptions.sourceMapCallback(id, result.map);
                    transformResult.map = JSON.parse(result.map);
                }
                return transformResult;
            }
            return undefined;
        }

Comparing hashes, it's always the same one so that's why udates are not getting refreshed in cache.

object-hash can't handle async functions for example. Maybe transformers are being wrapped in async by rollup. Or object-hash doesn't like something else.

The fix might be in object-hash or in rollup too.

Printing the properties of this.host I come up with this.host.transformers which contains the array of transformers I was looking for.

---

I did a quick test hashing only host.transformers (I don't know if other properties in host change between executions so... just in case) but still getting the same hash:

objectHash_1(host.transformers, this.hashOptions)

Not sure how this objectHash utility works with functions and so. Maybe some hashOptions we can play with?

Before getting deeper I would like some guidance from your part if possible @ezolenko

---

💡 created a repo example I'm playing with just case you want to use it to do some checks: https://github.com/oscard0m/rollup-plugin-typescript2-example

[oscard0m]

Here is a playground with object-hash and the problem I pointed in my previous comment

[oscard0m]

Hi @ezolenko! Just a friendly follow up on this issue :)

---

EDIT: Hi @ezolenko, I tried to find you in social networks but I did not. I would like to get as contributor of this project so we can move this forward and even participate in other issues or plans for this plugin. Would you be open for that option?

Thanks

[agilgur5]

EDIT: Hi @ezolenko, I tried to find you in social networks but I did not. I would like to get as contributor of this project so we can move this forward and even participate in other issues or plans for this plugin. Would you be open for that option?

Hi @oscard0m , sorry that no one's responded to you for over a year 😕 😞
ezolenko hasn't really had time to respond to issues and has just been trying to keep up with PRs.
I recently joined on as a maintainer, after being a contributor on/off for a few years, and have been going through literally all ~250 issues, making dozens of PRs etc etc etc, so I can help out here! Better late than never right? 😅

I also created the detailed contributing docs and one of my first major contributions was related to object-hash too (#203), so may be more suited to respond here as well.

💡 created a repo example I'm playing with just case you want to use it to do some checks: https://github.com/oscard0m/rollup-plugin-typescript2-example

This seems to 404 now 😕 . Can use the reproduction environment to make a quick repro accessible in the browser

Here is a playground with object-hash and the problem I pointed in my previous comment

Yeppp, reading your comments I suspected this was the problem as I had seen this behavior before when I was working on it. object-hash basically just uses toString() to hash against, which can include variables in it (and variables are just references too). So this means either changing the variable name is necessary to cache bust, or one can just directly in-line the transformers instead of using a variable.

For instance, in that CodeSandbox, if you in-line the functions directly into before instead of using the variable nodeRemover, you'll get different hashes.

So I think a bug/feature is needed in object-hash to properly support this, but it can be worked around in the meantime. I'll investigate object-hash a bit to try to remember where this code path is

[ezolenko]

Ugh, yeah, github sends too many emails and notifications, pings are easily getting lost there...

[oscard0m]

Hi @oscard0m, sorry that no one's responded to you for over a year 😕 😞
ezolenko hasn't really had time to respond to issues and has just been trying to keep up with PRs.
I recently joined on as a maintainer, after being a contributor on/off for a few years, and have been going through literally all ~250 issues, making dozens of PRs etc etc etc, so I can help out here! Better late than never right? 😅

Yes! Better late than never! Thanks for going through all the existing issues @agilgur5 ❤️

This seems to 404 now 😕 . Can use the reproduction environment to make a quick repro accessible in the browser

I'm sorry. I did a clean-up of my personal repositories and apparently deleted this example. I have it in my local machine so here's the reproduction in stackblitz:

https://stackblitz.com/edit/rpt2-repro-g7bbsj?file=README.md

So I think a bug/feature is needed in object-hash to properly support this, but it can be worked around in the meantime.

Yep, the workaround is what I ended up doing but the idea would be to centralize the function and import it as many times as we want so a fix in object-hash lib would be awesome. Opening an issue to them referencing this one to see if it would be possible: puleos/object-hash#118. Feel free to take part on it!

---

Ugh, yeah, GitHub sends too many emails and notifications, pings are easily getting lost there...

No problem. I can imagine how busy you are and how many notifications you can receive a day.

[agilgur5]

Appreciate the quick response after such a long lull @oscard0m ! And your patience and politeness/respect -- it really makes a difference to maintainers who often have to deal with lots of rudeness/passive-aggressiveness/toxicity in general while volunteering their time -- like I can't overstate enough how long a way that effort goes for maintainers' wellbeing ❤️

I'm sorry. I did a clean-up of my personal repositories and apparently deleted this example. I have it in my local machine so here's the reproduction in stackblitz:

https://stackblitz.com/edit/rpt2-repro-g7bbsj?file=README.md

No worries, totally understandable for links to not work after a year 😅
Thanks for providing a StackBlitz repro! A little different than expected (doesn't use the provided rollup.config.js and instead uses the Rollup API directly), but illustrates the problem nonetheless.

Opening an issue to them referencing this one to see if it would be possible: puleos/object-hash#118. Feel free to take part on it!

I investigated a decent amount and wrote about my findings there.
Basically transformer is itself a function, so its toString() is simply taken as the hash, but its contents happen to contain a variable, nodeRemover, which it can't really tell from a simple string 😕 . (not to mention, nodeRemover itself is a pretty complicated closure of closures).

As such, per my comment there, I think we may have a hit a "dead-end" in terms of what we might be able to expect from object-hash, in terms of feasibility, complexity, and maintenance, especially as similar issues have been closed out in the past.

So I think the workaround I proposed there might be the best possible solution to move forward with: rpt2 can implement a new option, additionalCacheDependencies (or again, something better named 😅 ), which can be any and is simply added to the hashed object as another property.
This would open up a decent number of workarounds for cache-busting for different use-cases, and so I think it could be a good feature to have in general.

As an example, could just add additionalCacheDependencies: nodeRemover as an option to rpt2 with this feature.

What do you think @oscard0m @ezolenko ?
I'd be open to a PR implementing that if @oscard0m is still interested in contributing 🙂

[agilgur5]

Ugh, yeah, github sends too many emails and notifications, pings are easily getting lost there...

@ezolenko, for reference, do you have a preferred method of contacting you?
I know I've similarly been unsure of how to solve a problem and asked for guidance in the past, before eventually figuring out how to solve the problem on my own.
Or, for instance, have wanted to coordinate a release. I've just been doing that by opening PRs and making PR comments for now, but not sure if that's preferred or optimal

Can shoot me an email if you'd rather not disclose contact methods in the comments

[oscard0m]

Thanks once again for the time, the energy, and for a so detailed answer here @agilgur5

Appreciate the quick response after such a long lull @oscard0m ! And your patience and politeness/respect -- it really makes a difference to maintainers who often have to deal with lots of rudeness/passive-aggressiveness/toxicity in general while volunteering their time -- like I can't overstate enough how long a way that effort goes for maintainers' wellbeing ❤️

Definitely, anything I can do to collaborate I will do it. I think being thankful for this project, your time, and your effort is the minimum we can do as users.

A little different than expected (doesn't use the provided rollup.config.js and instead uses the Rollup API directly), but illustrates the problem nonetheless.

This issue popped up to me in the previous company I was working for. They had a very wrong strange way to build their project I guess 🤷🏽.

What do you think @oscard0m @ezolenko?
I'd be open to a PR implementing that if @oscard0m is still interested in contributing 🙂

As I mentioned, this issue popped up in my previous company (in May 2020). The issue was that the project was not being rebuilt when some changes were applied (all this function and toString() drama we have been talking about). The big amount of time a colleague and I spent debugging was insane.

My desire was to arrive at a solution transparent to the rollup-plugin-typescript2 users where we don't have to be aware of how object-hash works to flag additionalCacheDependencies as an option with the parts of transpiled code susceptible to not be hashed correctly.

Your proposal implies being "conscious" of which parts can be hashed incorrectly. This can be unintuitive for users not aware of all this conversation and the implementation details of this plugin.

Considering this is the most feasible solution, as you pointed, I think it makes sense to draft a PR with your proposal.

My interest in contributing is not as strong as before. I'm not in the company I hit this bug with, so I'm not actively working with rollup-plugin-typescript2 anymore. Still, I think this is a great opportunity to work and learn together with this draft PR.

Would you like me to draft something and keep discussing there?

[agilgur5]

I think being thankful for this project, your time, and your effort is the minimum we can do as users.

💯 💯 💯 ❤️

They had a very wrong strange way to build their project I guess 🤷🏽.

I wanna say that rollup.config.js didn't always exist and the Rollup API was first (I'm not 100% sure about that since I started using it once it already hit 1.0.0), so that might be historical context there. Also Grunt/Gulp/other JS-based task runners used to be pretty popular, and so too were the JS APIs used with them, so could have been something like that. Legacy code 🤷

transparent design limitations

My desire was to arrive at a solution transparent to the rollup-plugin-typescript2 users

Your proposal implies being "conscious" of which parts can be hashed incorrectly. This can be unintuitive for users not aware of all this conversation and the implementation details of this plugin.

Absolutely agreed. I think we'd all definitely prefer that.

Unfortunately, I don't think we have much of any options, so I thought rather than just declaring this as blocked and unfixable, we could implement some workaround for it.
Of course here in rpt2, clean: true, is always possible to disable the cache, but as that can be a significant deoptimization for larger projects, I thought that another workaround that leaves caching intact such as the proposed additionalCacheDependencies option, might the best we can get.

Also, as that'll be an option listed in the docs, it can also act as a more of a hint to the user that the cache is not infallible (which the clean and objectHashIgnoreUnknownHack options suggest already, as well as the Contributing docs), and, in this case, can not only have bugs in the implementation, but can be literally unable to properly cache some things, like the dynamic behavior of functions.

Related: I left another comment just now in the linked object-hash issue, and with some even deeper investigation, this is practically impossible to do transparently.
The best we could get is running a Babel macro on the rollup.config.js itself or an ESLint rule, but those are not transparent either. Not even Babel itself does that.
From that perspective, an option like additionalCacheDependencies (similar to React's useCallback hook's dependencies) is significantly simpler to use, given only those options to compare against.

warnings

• What do you think about adding a warning when cache is used so users notice about it with default verbosity level?

Getting back to the very opening of this issue, given our current knowledge and those comments, it might also be a good idea to output a warning when a "dynamic" option that can't be wholly cached is used, such as transformers in this case, or any other function options (like sourceMapCallback). That warning can point to the docs for additionalCacheDependencies which would reference this issue.
That warning might dovetail well with the options schema checking feature (#312) I'd like to add as well.

We could add an info when using the cache in general, though that's not the default verbosity level. At the default verbosity level, that might be output all the time for most users, so might be controversial (if you've ever tried to get all your logs to be silent in CI/CD, you know how annoying that can get; while setting verbosity: 0 can workaround that, that might not be ideal either).

• Is there any historical reason to set clean: false as default? It sounds like the default configuration should be without """magic""" on top right?

Otherwise, anything else can be considered a cache invalidation bug, which, well, is known as one of the two hardest problems in Computer Science for a reason 😅

Caches are pretty ubiquitous in computing, e.g. DNS, CDNs, write buffers, L1/L2/L3 caches etc etc etc etc, and they default to "on" pretty often (and other libraries in the JS/TS ecosystem do as well), so I'm not sure this behavior is a departure from the norm in that sense, but I didn't implement the defaults to know the original rationale. It would be a de-opt for larger projects by default too, as mentioned above.

The docs, contributing docs, issues, etc, do mention the cache, debug verbosity, etc, so I don't think it should be altogether that surprising to users either. There aren't altogether that many cache issues either, and I think in nearly all situations, users are aware they can use clean: true as a workaround too (but would prefer the cache be fixed of course).

But, of course, as with most things in software engineering, there's trade-offs in either direction and I could honestly defend either as a default.

contributing

My interest in contributing is not as strong as before. I'm not in the company I hit this bug with, so I'm not actively working with rollup-plugin-typescript2 anymore.

Yup, totally expected that given the big gap of time, so no offense taken there.
I could implement an additionalCacheDependencies option relatively quickly, but figured offering that up as a possible contribution would be better stewardship of the community, especially in this case where a community member had already expressed interest in the past 🙂

Still, I think this is a great opportunity to work and learn together with this draft PR.

Would you like me to draft something and keep discussing there?

That sounds like a plan to me! Though @ezolenko has final say, so don't want to overstep there, but I imagine this feature would be welcomed.
And it has a relatively small scope in terms of parts of the codebase it changes, which makes it more accessible and easier to approve for newer contributors. The most complicated part is probably getting the wording right for the docs and the naming of the option (in my opinion at least, obviously I know this codebase pretty well though, unlike most contributors) 😅

Separately, I'm also open to a PR (or PRs) with regard to additional warnings as well.

[oscard0m]

That sounds like a plan to me! Though @ezolenko has final say, so don't want to overstep there, but I imagine this feature would be welcomed.

Cool! Let's wait for @ezolenko and I will draft-pr something then :)

Separately, I'm also open to a PR (or PRs) with regard to additional warnings as well

Do you want me to create a new issue for this (and draft a PR for it)? @agilgur5

[agilgur5]

Do you want me to create a new issue for this (and draft a PR for it)? @agilgur5

I don't think a new issue is necessary (since this issue details the rationale for more warnings). PR welcome 👍

[agilgur5]

additionalCacheDependencies (or again, something better named 😅 )

I did think of a better name for this, or, at least, a shorter name: extraCacheKeys.

"cache key" is already mentioned in the docs for objectHashIgnoreUnknownHack already, and this impacts the same area of the code dealing with the hash / object-hash.

still open to better names though 😅

also cc @ezolenko still waiting on a 👍 / 👎 here on the proposal! this is the issue I was trying to bring your attention to

[ezolenko]

@ezolenko, for reference, do you have a preferred method of contacting you?

@agilgur5 yeah, github is sending too many emails all of the same kind (especially lately :D), so I often miss pings here. I'll play around with notification settings to limit those to PRs and direct pings, but you can also send direct email to zolenkoe at gmail.

Getting back to the very opening of this issue, given our current knowledge and those comments, it might also be a good idea to output a warning when a "dynamic" option that can't be wholly cached is used

I think warning is not strong enough -- maybe we should disable cache completely in those cases and output a warning about that (unless user provided extraCacheKeys option, we then leave everything up to the user). Then default behavior for users who don't read warnings would be slower, but more correct compilation.

[agilgur5]

I think warning is not strong enough -- maybe we should disable cache completely in those cases and output a warning about that (unless user provided extraCacheKeys callback, we then leave everything up to the user). Then default behavior for users who don't read warnings would be slower, but more correct compilation.

Mm that's a good point and would definitely help users avoid this kind of issue or be more aware of it since they have to opt-in if using a "dynamic" option (I believe the only two are sourceMapCallback and transformers).
We'll have to word the error message and docs pretty carefully for clarity on this complex issue, but we can iterate on that separately (and/or over time).

@oscard0m think you're clear to work on this! 🙂

[agilgur5]

@oscard0m just wanted to check in and see if you were still working on a PR for this issue?

[oscard0m]

@agilgur5 sorry for the late answer. I just drafted a small PR. We can keep the conversation there so we assure we move this forward: #420