rollup-plugin-typescript2: Cache is not cleared when `transformer` changes

@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 😄), 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.

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.

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?

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

@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: https://github.com/ezolenko/rollup-plugin-typescript2/pull/420

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! 🙂

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

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 🙂

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: https://github.com/puleos/object-hash/issues/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.

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

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.

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.