Repository navigation
fix(core): remove all toplevel property accesses - #30239
filipesilva wants to merge 5 commits into
Conversation
|
You can preview 81f4228 at https://pr30239-81f4228.ngbuilds.io/. |
|
You can preview 2b5a2a3 at https://pr30239-2b5a2a3.ngbuilds.io/. |
|
You can preview 19f7a04 at https://pr30239-19f7a04.ngbuilds.io/. |
| "uncompressed": { | ||
| "runtime": 1497, | ||
| "main": 164945, | ||
| "main": 166696, |
There was a problem hiding this comment.
Vaguely surprised this went up a bit. I guess it's because of how the IIFEs increase code, especially when transpiled to ES5.
For instance
// original
const CORE_TOKENS = {
'ApplicationRef': ApplicationRef,
'NgZone': NgZone,
};
// transpiled
var CORE_TOKENS = {
'ApplicationRef': ApplicationRef,
'NgZone': NgZone,
};
versus
// modified with IIFE
const CORE_TOKENS = (() => ({
'ApplicationRef': 'ApplicationRef',
'NgZone': 'NgZone',
}))();
// transpiled
var CORE_TOKENS = (function () { return ({
'ApplicationRef': 'ApplicationRef',
'NgZone': 'NgZone',
}); })();
| */ | ||
| // It's ok to access a property on Promise because it's a native global. | ||
| // tslint:disable-next-line:no-toplevel-property-access | ||
| const _CLEAN_PROMISE = Promise.resolve(null); |
There was a problem hiding this comment.
The tslint rule should ignore known globals, or even have a ignore list.
| (() => ({ | ||
| 'ɵɵdefineBase': r3.ɵɵdefineBase, | ||
| 'ɵɵdefineComponent': r3.ɵɵdefineComponent, | ||
| 'ɵɵdefineDirective': r3.ɵɵdefineDirective, |
There was a problem hiding this comment.
This map strikes me as worrisome. Won't everything be retained if angularCoreEnv is used anywhere?
| "name": "FLAGS" | ||
| }, | ||
| { | ||
| "name": "FactoryPrototype" |
There was a problem hiding this comment.
Looks like this was being retained unnecessarily.
|
You can preview 71e1a02 at https://pr30239-71e1a02.ngbuilds.io/. |
| */ | ||
|
|
||
| import * as core from '@angular/core'; | ||
| import {APP_INITIALIZER, ApplicationRef, DebugNode, NgProbeToken, NgZone, Optional, Provider, getDebugNode} from '@angular/core'; |
There was a problem hiding this comment.
Not sure why this file was importing core as a namespace.
| */ | ||
| // It's ok to access a property on Promise because it's a native global. | ||
| // tslint:disable-next-line:no-toplevel-property-access | ||
| const _CLEAN_PROMISE = Promise.resolve(null); |
There was a problem hiding this comment.
There's also a few of these global resolved promises just hanging around, they all seem the same.
|
You can preview 56b96e8 at https://pr30239-56b96e8.ngbuilds.io/. |
|
The TSLint rule identified some toplevel prop access on rxjs too, that I changed in https://github.com/filipesilva/rxjs/commit/4041f950dfd64e03623a69e14c67f79d353902ea, built and published to github as https://github.com/filipesilva/rxjs-6.4.0-without-toplevel-prop-assignment |
| "name": "EMPTY_OBJ" | ||
| }, | ||
| { | ||
| "name": "EmptyErrorImpl" |
There was a problem hiding this comment.
These three seem to be leftover due to the prop access/assignment in https://github.com/filipesilva/rxjs/commit/4041f950dfd64e03623a69e14c67f79d353902ea#diff-7aec05e21b282d021d49c39873cdff07R8.
|
You can preview 3888998 at https://pr30239-3888998.ngbuilds.io/. |
|
You can preview 1a116a9 at https://pr30239-1a116a9.ngbuilds.io/. |
|
With the last commit, the one that uses This is still an increase of ~2.5k, but much less than the ~14k increase we saw before. |
Isn't it an increase of 230 bytes, or 1.6%? |
|
@kzc only if count it correctly, which I didn't :D Thanks for noticing that! |
|
You can preview 1f1a835 at https://pr30239-1f1a835.ngbuilds.io/. |
|
You can preview d216abb at https://pr30239-d216abb.ngbuilds.io/. |
|
Closing this PR as is to keep the comments and experiment results. #29329 will include this PR and in-repo tests to validate the changes work. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Followup to angular/angular-cli#14316