"Maximum call stack size exceeded." w/ ember && ember-data 2.11 beta

[workmanw]

There are quite a few test failures with ember and ember-data beta. They all seem to be max call stack issues. I'll investigate in the next few weeks as I have time, but feel free to weigh in if anyone has thoughts.

    superWrapper@http://localhost:7357/assets/vendor.js:50272:27
    lookupSerializer@http://localhost:7357/assets/vendor.js:84257:32
    serializerFor@http://localhost:7357/assets/vendor.js:94432:37
    ....
    superWrapper@http://localhost:7357/assets/vendor.js:50272:27
    lookupSerializer@http://localhost:7357/assets/vendor.js:84257:32
    serializerFor@http://localhost:7357/assets/vendor.js:94432:37
    superWrapper@http://localhost:7357/assets/vendor.js:50272:27
    lookupSerializer@http://localhost:7357/assets/vendor.js:84257:32
    serializerFor@http://localhost:7357/assets/vendor.js:94432:37
    superWrapper@http://localhost:7357/assets/vendor.js:50272:27
    http://localhost:7357/assets/tests.js:3853:34
    runTest@http://localhost:7357/assets/test-support.js:3255:32
    run@http://localhost:7357/assets/test-support.js:3240:11
    http://localhost:7357/assets/test-support.js:3382:14
    process@http://localhost:7357/assets/test-support.js:3041:24
    begin@http://localhost:7357/assets/test-support.js:3023:9
    http://localhost:7357/assets/test-support.js:3083:9
message: >
    Died on test #1 http://localhost:7357/assets/tests.js:3849:19
    exports@http://localhost:7357/assets/vendor.js:132:37
    requireModule@http://localhost:7357/assets/vendor.js:32:25
    require@http://localhost:7357/assets/test-support.js:7085:14
    loadModules@http://localhost:7357/assets/test-support.js:7077:21
    load@http://localhost:7357/assets/test-support.js:7107:33
    http://localhost:7357/assets/test-support.js:6990:22: Maximum call stack size exceeded.
Log: |

[DingoEatingFuzz]

    ....
    superWrapper@http://localhost:7357/assets/vendor.js:50272:27
    lookupSerializer@http://localhost:7357/assets/vendor.js:84257:32
    serializerFor@http://localhost:7357/assets/vendor.js:94432:37
    superWrapper@http://localhost:7357/assets/vendor.js:50272:27
    lookupSerializer@http://localhost:7357/assets/vendor.js:84257:32
    serializerFor@http://localhost:7357/assets/vendor.js:94432:37
    superWrapper@http://localhost:7357/assets/vendor.js:50272:27

Looks like a really tight cycle at the very least.

[workmanw]

So this guy is definitely the culprit: emberjs/data@36bc08a . I've got some cycles to spend right now, I'll get this fixed up. I believe @runspired also has coming changes for ember-data, I'll see if I can investigate them and figure out what the impact is for this addon. Stay tuned.

---

EDIT: The trick seems to be that ember-data now uses it's own ContainerInstanceCache that no longer supports fallback adapters. So we may no longer be able to support the previous way of looking up a -fragment adapter. This could require a breaking change for this addon to resolve. Need to relearn some of the DS internals to say for sure.

[runspired]

@workmanw still supports, but the fallback logic moved into the cache (which already was a thing)

[workmanw]

@runspired the problem is that previously this addon could provide it's own fallback because ContainerInstanceCache.get would accept that as a third argument. Unfortunately that is no longer the case. As you mentioned, the fallback handling now completely resides inside the cache. And because it's an ES6 class, AFAIK, we are unable to monkey patch this.

Do you have any thoughts on a modification that could be made to allow a fallback to be passed in to get as third argument once again.

[runspired]

I'd originally left the fallback logic outside of the cache but @igorT and I didn't like that this made the cache and the store need to be bound tightly together. Can you point me to what this fallback is / does?

[runspired]

@workmanw looking through this library's code, I dont see anywhere you make calls into adapterFor.

[workmanw]

:( I'm sorry I said adapter ... I meant to say serializer. See here: https://github.com/lytics/ember-data-model-fragments/blob/master/addon/ext.js#L96-L102

You can see in that case the store is being monkey patched to enable Fragments to have their own default serializer. The design is that just as the store proper has it's own "application serializer" for models, fragments too can have a default fragment serializer. It's my impression that many apps, mine included, just use the "application serializer" as their default fragment serializer, but I definitely can't say that is definitive.

[runspired]

It is likely that you can monkey patch this method:

https://github.com/emberjs/data/blob/36bc08ab942d0a4113178f1c131c9e7024f0c995/addon/-private/system/store/container-instance-cache.js#L64-L75

However I'm in favor of there being a public API for the fallbacks. If you'd open up an issue and are willing to work on an RFC I think this is something we can add and get in before beta goes to stable.

[workmanw]

@runspired I think monkey patching _fallbacksFor could be problematic because you don't really know what type if the preferredKey is for a fragment or standard model at that point.

It sounds like you're suggesting a public API for all of ember-data to provide a public mechanism for fallback types. Is that right? Or were you suggesting a public mechanism just for the ContainerInstanceCache itself? The reason I ask is one is definitely significantly more work than the other.

[runspired]

@workmanw I think the existing monkey patch you have could be ported fairly seamlessly.

let instanceCache = store._instanceCache;
let _super = instanceCache.prototype._fallbacksFor;
instanceCache.prototype._fallbacksFor = function _monkeyPatchedFallbacksFor(namespace, preferredKey) {
  if (namespace === 'serializer') {
    let model = this._store._modelFor(preferredKey);
    
    if (model && Fragment.detect(model)) {
      return [
        '-fragment',
        '-default'
      ];
    }
  }
  
   return _super(namespace, preferredKey);
};

[runspired]

It sounds like you're suggesting a public API for all of ember-data to provide a public mechanism for fallback types

This is correct, I suspect it would be something along the lines of:

store.registerContainerFallback(cb)

Although the more I think about it, we kinda want to disallow or discourage customization of the fallbacks, this should be opened as an issue on the ember-data repo for more people to contribute to the discussion. I want to support fragments but I don't particularly want to build APIs that are really only there for fragments specifically.

[workmanw]

@runspired D'oh! That would totally work. I just got it in my head it needed to be an argument. Thanks a lot for the back and forth here.

Although the more I think about it, we kinda want to disallow or discourage customization of the fallbacks ...

Honestly, I have to agree as well. I think it would be hard to explain the need for this outside the context of fragments or some other similar addon.

In the year or so that I've been helping out with this addon I've tried to come up with ways to simplify the relationship, even if just for cognitive reasons. It's super tough because ember-data doesn't want to, and shouldn't have to, keep it's private APIs stable. On the other side, enough people use the addon, that value has clearly demonstrated. I think one of the brittle aspects of this addon is that we end up monkey patching things. If there was a more clean way to extend the store and the model classes, and those classes offered delegate like hooks (e.g. changedAttributes) it could be an improvement. But that's just kind of a brain dump.

Anyways, thanks again!

[allthesignals]

I'm seeing this issue on the following versions:

    "ember-source": "~3.7.3",
    "ember-data": "~3.7.0",
    "ember-data-model-fragments": "^4.0.0",

Here's our log test log: https://circleci.com/gh/NYCPlanning/ceqr-app/1765

This issue appears to be closed... maybe it is resurfacing? Or maybe it's unrelated to model fragments.

Here's a trace:

Uncaught RangeError: Maximum call stack size exceeded
at Arguments.values ()
at Registry.resolve (vendor.js:16445)
at has (vendor.js:16826)
at Registry.has (vendor.js:16527)
at Registry.proto.validateInjections (vendor.js:16776)
at processInjections (vendor.js:16148)
at buildInjections (vendor.js:16172)
at injectionsFor (vendor.js:16184)
at FactoryManager.create (vendor.js:16232)
at instantiateFactory (vendor.js:16134)

Edit:
After undoing everything and slowly reintroducing, I'm finding that simply installing the addon triggers the maximum call stack issue.