sandydoo
commented
3 years ago TL;DR
DummyPackage and Package do not share the same typescript-memoize dependency in addons that use yarn.
This causes @Memoize to generate conflicting/overlapping caching keys, as it uses a top-level, module-scoped counter to increment these keys.
This is an issue for DummyPackage and Package, since they both use @Memoize, are both defined in separate packages, and are related by class inheritance. They end up trying to overwrite each other’s caches.
Possible solutions:
- Make
typescript-memoizea singleton dependency? Adding it as a top-level dependency to Embroider seems to work. Are there downsides to this I‘m not aware of? - ~~Propose a change to
typescript-memoizeto use a different caching scheme? Perhaps one that uses the property key, instead of a “global” counter. Something like this should fix the issue: https://github.com/sandydoo/typescript-memoize/commit/31527dbafb7fb732219fa8bd88edd4d81549b042~~
simonihmig
muziejus
I was playing around with
@embroider/macrosand noticed that my addon’s dummy app wouldn’t build with Embroider anymore. Here’s the error it throws:So I started looking into this…
In a nutshell, we’re trying to get
packageJSONinDummyPackage. So,@Memoizeis going to run the getter function and then cache the value tothis.__memoized_value_2.https://github.com/darrylhodgins/typescript-memoize/blob/6c215d5fe37e2149c0463cfee6a7b5ad908951bd/src/memoize-decorator.ts#L91-L97
The problem is that in the time that we’re “computing” the value, fetching
nonResolvableDeps,internalPackageJSONand the like, the contextthisseems to change.@Memoizethen tries to do anObject.define, using the key__memoized_value_2as it initially intended, but that property already exists onthis.I’ve found two ways to make the bug stop:
Remove at least one
@Memoizefrom eitherDummyPackageor the parentPackage.PackageJSON,internalPackageJSON, ornonResolvableDeps. Doesn’t matter which. This seems to make the cache counters intypescript-memoizehappy in this particular case.Remove
@embroider/macrosfrom the addon’s dependencies. This I don’t understand. It happily builds without it. What? Why? How? Any ideas why this would work? Sometimes, you have to nukenode_modulesto get it to build again.This is where I’m at.
I tried recreating a similar scenario in the tests for
typescript-memoize. Their tests don’t test class inheritance with a bunch of@Memoizes. I figured it’d be worth a shot, but it hasn’t panned out.I also started peppering the code with some classic
console.logs, so here’s a rough call stack of what happens right up until the crash:I hope this helps somewhat, or at least sparks some ideas of where to look. 👍
Steps to reproduce
I couldn’t reliably reproduce this in a fresh repo, but I’ve made a repro branch for you on
ember-google-maps.git switch repro-embroider-memoize-bugember bVersions
Updates
1. The counters overlap
On
DummyPackage:__memoize_value_1<==internalPackageJSON__memoize_value_2<==nonResolvableDepsand then we try to overwrite__memoize_value_2withpackageJSON.So the counters match the order of the getters in each class:
Is this a module thing? Is it because
DummyPackageandPackageare defined in different packages? Each has it’s own “instance” oftypescript-memoize, with separate counters, and it so happens that they overlap betweenDummyPackageandPackage?2. It‘s a module issue
Using
yarn link typescript-memoizeto a local repo fixes the issue. So it really is a module issue!3. It’s a module and YARN thing
Using
npm installfixes the issue.npmseems to deduplicate and hoisttypescript-memoizeas a top-level module, whileyarnhoists, but each package still gets its own version. So what do you do if you want to useyarn?4. It’s a module +
yarn+ember-auto-importthingIt seems
ember-auto-importis also playing a role here. It builds if you remove it. Shenanigans! Addingtypescript-memoizeto dependencies on the root embroider workspace also works.