Skip to content

Commit

Permalink
fix(store): do not run Promise.then within synchronous tests when d…
Browse files Browse the repository at this point in the history
…ecorating factory (ngxs#1753)
  • Loading branch information
arturovt authored May 22, 2021
1 parent 8f72cfe commit 66c2af7
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 14 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ $ npm install @ngxs/store@dev
```

- Fix: Do not re-use the global `Store` instance between different apps [#1740](https://github.com/ngxs/store/pull/1740)
- Fix: Do not run `Promise.then` within synchronous tests when decorating factory [#1753](https://github.com/ngxs/store/pull/1753)
- Performance: Tree-shake errors and warnings [#1732](https://github.com/ngxs/store/pull/1732)
- Performance: Router Plugin - Tree-shake `isAngularInTestMode()` [#1738](https://github.com/ngxs/store/pull/1738)
- Performance: Tree-shake `isAngularInTestMode()` [#1739](https://github.com/ngxs/store/pull/1739)
Expand Down
4 changes: 2 additions & 2 deletions bundlesize.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@
"path": "./@ngxs/store/fesm2015/ngxs-store-internals.js",
"package": "@ngxs/store/internals",
"target": "es2015",
"maxSize": "10.85KB",
"maxSize": "11.75KB",
"compression": "none"
},
{
"path": "./@ngxs/store/fesm5/ngxs-store-internals.js",
"package": "@ngxs/store/internals",
"target": "es5",
"maxSize": "11.80KB",
"maxSize": "12.70KB",
"compression": "none"
},
{
Expand Down
2 changes: 1 addition & 1 deletion integrations/hello-world-ng12-ivy/bundlesize.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
{
"path": "./dist-integration/main.*.js",
"target": "es2015",
"maxSize": "235.72 kB",
"maxSize": "236 kB",
"compression": "none"
}
]
Expand Down
45 changes: 37 additions & 8 deletions packages/store/internals/src/decorator-injector-adapter.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
import { InjectionToken, Injector, INJECTOR, Type, ɵɵdirectiveInject } from '@angular/core';
import {
InjectionToken,
Injector,
INJECTOR,
Type,
ɵɵdirectiveInject,
ɵglobal
} from '@angular/core';

// Will be provided through Terser global definitions by Angular CLI
// during the production build. This is how Angular does tree-shaking internally.
declare const ngDevMode: boolean;

// Angular doesn't export `NG_FACTORY_DEF`.
const NG_FACTORY_DEF = 'ɵfac';
Expand All @@ -19,16 +30,14 @@ export function ensureLocalInjectorCaptured(target: Object): void {
// Means we're in AOT mode.
if (typeof constructor[NG_FACTORY_DEF] === 'function') {
decorateFactory(constructor);
} else {
} else if (ngDevMode) {
// We're running in JIT mode and that means we're not able to get the compiled definition
// on the class inside the property decorator during the current message loop tick. We have
// to wait for the next message loop tick. Note that this is safe since this Promise will be
// resolved even before the `APP_INITIALIZER` is resolved.
// The below code also will be executed only in development mode, since it's never recommended
// to use the JIT compiler in production mode (by setting "aot: false").
Promise.resolve().then(() => {
decorateFactory(constructor);
});
decorateFactoryLater(constructor);
}

target.constructor.prototype[FactoryHasBeenDecorated] = true;
Expand Down Expand Up @@ -82,12 +91,32 @@ function decorateFactory(constructor: ConstructorWithDefinitionAndFactory): void
}
}

// We could've used `ɵɵFactoryDef` but we try to be backward compatible,
function decorateFactoryLater(constructor: ConstructorWithDefinitionAndFactory): void {
// This function actually will be tree-shaken away when building for production since it's guarded with `ngDevMode`.
// We're having the `try-catch` here because of the `SyncTestZoneSpec`, which throws
// an error when micro or macrotask is used within a synchronous test. E.g. `Cannot call
// Promise.then from within a sync test`.
try {
Promise.resolve().then(() => {
decorateFactory(constructor);
});
} catch {
// This is kind of a "hack", but we try to be backwards-compatible,
// tho this `catch` block will only be executed when tests are run with Jasmine or Jest.
ɵglobal.process &&
ɵglobal.process.nextTick &&
ɵglobal.process.nextTick(() => {
decorateFactory(constructor);
});
}
}

// We could've used `ɵɵFactoryDef` but we try to be backwards-compatible,
// since it's not exported in older Angular versions.
type Factory = () => PrivateInstance;

// We could've used `ɵɵInjectableDef`, `ɵɵPipeDef`, etc. We try to be backward
// compatible since they're not exported in older Angular versions.
// We could've used `ɵɵInjectableDef`, `ɵɵPipeDef`, etc. We try to be backwards-compatible
// since they're not exported in older Angular versions.
interface Definition {
factory: Factory | null;
}
Expand Down
7 changes: 4 additions & 3 deletions setupJest.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// import 'jest-preset-angular'; // commented out due to issue in latest jest-preset-angular
import { ɵglobal } from '@angular/core';

const CI = process.env['CI'] === 'true';

Expand Down Expand Up @@ -48,6 +49,6 @@ if (CI) {
});
}

// Most of our tests are not aware of the `ngDevMode` Ivy and still rely on the old
// behavior (as it was in the View Engine).
((global as unknown) as { ngDevMode: undefined }).ngDevMode = undefined;
if (typeof ɵglobal.ngDevMode === 'undefined') {
ɵglobal.ngDevMode = true;
}

0 comments on commit 66c2af7

Please sign in to comment.