Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

fix(core): fix #989, remove unuse code, use shorter name to reduce bundle size #990

Merged
merged 7 commits into from
Jan 10, 2018

Conversation

JiaLiPassion
Copy link
Collaborator

@JiaLiPassion JiaLiPassion commented Jan 6, 2018

fix #989.

  1. remove unused code

  2. move getUserMedia and electron related code to standalone files.

  3. use shorter variable and function name to reduce bundle size.
    now the size of zone.min.js will be 38510 or 38955 bytes. (the size will change when I rebuild)

  4. add file size check logic in travis build.

  5. change func.apply to func.call for better performance.

now I set a limitation of zone.min.js to 40000 bytes.

@mhevery , please review the idea is ok or not. thank you!

@JiaLiPassion JiaLiPassion changed the title fix(core): fix #989, remove unuse code, use shorter name to reduce bundle size WIP(core): fix #989, remove unuse code, use shorter name to reduce bundle size Jan 7, 2018
@JiaLiPassion JiaLiPassion changed the title WIP(core): fix #989, remove unuse code, use shorter name to reduce bundle size fix(core): fix #989, remove unuse code, use shorter name to reduce bundle size Jan 7, 2018
@@ -11,50 +11,51 @@
* @suppress {undefinedVars,globalThis,missingRequire}
*/

// issue #989, to reduce bundle size, use short name
export const a = Object.getOwnPropertyDescriptor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need to have a single char letters here. You can have long names, they will get renamed using uglify. Single letters are hard on readability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out this one, I have misunderstanding about uglfiyjs, I have changed the variable name to long names.

export const k = 'true';
export const l = 'false';
export const m = '__zone_symbol__';
export const n = 'function';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think typeof x == 'string is faster than typeof x == STRING_CONST. That is because in one case the jitter can create the check code which directly checks type to be string, in tho other case it has to generate a more complex code which could be actual type and than compare it to the constant. I would revert this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I have reverted all typeof x === parts.

if (canPatchViaPropertyDescriptor()) {
const ignoreProperties: IgnoreProperty[] = _global.__Zone_ignore_on_properties;
// for browsers that we can patch the descriptor: Chrome & Firefox
if (isBrowser) {
const w: any = window;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local vars get renamed. Calling it w makes it hard to read. Could we call it window and that way we don't have to change the code bellow? I know window = window would not work, but something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you are right, I have changed the variable to internalWindow.

lib/zone.ts Outdated
performanceMeasure('Zone', 'Zone');
const z: any = Zone.prototype;
z.w = z.wrap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that we don't pollute the Zone public API with these short names. I realized they are not part of .d.ts but they will show up in debugger.

How about this idea
Zone.current could be replaced with getZoneCurrent() which is declared in util file. The uglify will rename the getZoneCurrent to something short like x.

Zone.current.wrap(...) could be wrapWithCurrentZone(...)

and so on. What do you think?

Copy link
Collaborator Author

@JiaLiPassion JiaLiPassion Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a better solution, I created wrapWithCurrentZone and scheduleMacroTaskWithCurrentZone helper method.

lib/zone.ts Outdated
z.si = z.scheduleMicroTask;
z.se = z.scheduleEventTask;
z.ct = z.cancelTask;
(Zone as any).l = Zone.__load_patch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing here, helpers in util.ts can hide the long name without loosing readability

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

Copy link

@alfaproject alfaproject left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is good for the maintainability of the project, and most of the things you changed will minify/compress just fine and with better results.

@@ -41,7 +38,8 @@ export function propertyPatch() {
};

Object.create = <any>function(obj: any, proto: any) {
if (typeof proto === OBJECT && !Object.isFrozen(proto)) {
// o is 'object' string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I have reverted this one, thank you!

@@ -11,50 +11,72 @@
* @suppress {undefinedVars,globalThis,missingRequire}
*/

// issue #989, to reduce bundle size, use short name

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't do the job of a compressor.
In fact, you are probably adding entropy.

P.S.: all of these extra imports/exports add up to browser parsing/interpreting time as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, I have misunderstanding about uglifyjs, I have modified the PR.

@JiaLiPassion JiaLiPassion force-pushed the size branch 2 times, most recently from e6eb346 to d7ef237 Compare January 9, 2018 06:37
@mhevery mhevery merged commit 8293c37 into angular:master Jan 10, 2018
@IgorMinar
Copy link
Contributor

awesome! thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.4kb size regressions between 0.8.16 and 0.8.19
5 participants