Skip to content

Commit

Permalink
Correctly associate props with factory and owner in FactoryManager
Browse files Browse the repository at this point in the history
Previously, we stomped the `props` binding with an `Object.assign()`,
which meant that the original empty props object would get GC'd after
the end of the method and the item passed into the class created at the
end of the `FactoryManager.create` call would be a *different* object,
which does *not* have the factory or owner associations.

Fixes #20023
  • Loading branch information
chriskrycho committed Mar 16, 2022
1 parent d8f8266 commit 0b154e7
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 10 deletions.
6 changes: 1 addition & 5 deletions packages/@ember/-internals/container/lib/container.ts
Expand Up @@ -504,14 +504,10 @@ export class FactoryManager<T, C extends FactoryClass | object = FactoryClass> {
);
}

let props = {};
let props = options ? { ...options } : {};
setOwner(props, container.owner!);
setFactoryFor(props, this);

if (options !== undefined) {
props = Object.assign({}, props, options);
}

if (DEBUG) {
let lazyInjections;
let validationCache = this.container.validationCache;
Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/-internals/routing/lib/services/routing.ts
Expand Up @@ -12,7 +12,7 @@ import { Route } from '../..';
import EmberRouter from '../system/router';
import RouterState from '../system/router_state';

const ROUTER = (symbol('ROUTER') as unknown) as string;
const ROUTER = symbol('ROUTER');

/**
The Routing service is used by LinkTo, and provides facilities for
Expand Down
10 changes: 6 additions & 4 deletions packages/@ember/-internals/routing/lib/system/router.ts
Expand Up @@ -341,7 +341,7 @@ class EmberRouter<R extends Route = Route> extends EmberObject.extend(Evented) i

assert('Route is unexpectedly missing an owner', routeOwner);

let route = routeOwner.lookup(fullRouteName) as R;
let route = routeOwner.lookup(fullRouteName) as R | undefined;

if (seen[name]) {
assert('seen routes should exist', route);
Expand All @@ -351,6 +351,8 @@ class EmberRouter<R extends Route = Route> extends EmberObject.extend(Evented) i
seen[name] = true;

if (!route) {
// SAFETY: this is configured in `commonSetupRegistry` in the
// `@ember/application/lib` package.
let DefaultRoute: any = routeOwner.factoryFor('route:basic')!.class;
routeOwner.register(fullRouteName, DefaultRoute.extend());
route = routeOwner.lookup(fullRouteName) as R;
Expand All @@ -362,15 +364,15 @@ class EmberRouter<R extends Route = Route> extends EmberObject.extend(Evented) i
}
}

route!._setRouteName(routeName);
route._setRouteName(routeName);

if (engineInfo && !hasDefaultSerialize(route!)) {
if (engineInfo && !hasDefaultSerialize(route)) {
throw new Error(
'Defining a custom serialize method on an Engine route is not supported.'
);
}

return route!;
return route;
}

getSerializer(name: string) {
Expand Down

0 comments on commit 0b154e7

Please sign in to comment.