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

add OriginalDelegate prop to Function::toString#993

Merged
mhevery merged 1 commit intoangular:masterfrom
gdh1995:hook-toString
Feb 8, 2018
Merged

add OriginalDelegate prop to Function::toString#993
mhevery merged 1 commit intoangular:masterfrom
gdh1995:hook-toString

Conversation

@gdh1995
Copy link
Copy Markdown
Contributor

@gdh1995 gdh1995 commented Jan 13, 2018

This PR complements PR #686 , making Function.prototype.toString.toString() also returns "function toString() { [native code] }", which should be a little safer.

Update: Function.prototype.toString.call(Function.prototype.toString) will return [native code], too.

@JiaLiPassion
Copy link
Copy Markdown
Collaborator

JiaLiPassion commented Jan 13, 2018

@gdh1995 , thank you for making the PR, could you add a test case in https://github.com/angular/zone.js/blob/master/test/common/toString.spec.ts to check Function.prototype.toString.toString() equals to native code.

@gdh1995
Copy link
Copy Markdown
Contributor Author

gdh1995 commented Jan 14, 2018

@JiaLiPassion Thanks, I've added a new test.

Comment thread lib/common/to-string.ts
}
return originalFunctionToString.apply(this, arguments);
};
(newFunctionToString as any)[ORIGINAL_DELEGATE_SYMBOL] = originalFunctionToString;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

current version, we attached originalFunctionToString to Zone, and if we attached it to Function.prototype, we don't need to attached it to Zone any more.

so please just change this line https://github.com/gdh1995/zone.js/blob/b98f284030a4296a0e476728829298515edc3b0d/lib/common/to-string.ts#L14

const originalFunctionToString = Function.prototype[ORIGINAL_DELEGATE_SYMBOL] =
      Function.prototype.toString;

@JiaLiPassion
Copy link
Copy Markdown
Collaborator

@gdh1995 , I just leave a comment, we don't need to keep original toString in two places, and please squash your commits by git rebase.

@gdh1995
Copy link
Copy Markdown
Contributor Author

gdh1995 commented Jan 14, 2018

Thanks. I've rebased those commits.

Comment thread lib/common/to-string.ts Outdated
@@ -11,13 +11,12 @@ import {zoneSymbol} from './utils';
// look like native function
Zone.__load_patch('toString', (global: any, Zone: ZoneType) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please also remove Zone: ZoneType here to reduce the bundle size.

@JiaLiPassion
Copy link
Copy Markdown
Collaborator

@gdh1995 , thank you, please also remove the unused Zone in parameter list.

This makes `Function.prototype.toString.toString()` also returns `"function toString() { [native code] }"`, which should be a little safer.

Squashed:
store originalFunctionToString only in one place
add a test to Function::toString
@gdh1995
Copy link
Copy Markdown
Contributor Author

gdh1995 commented Jan 15, 2018

The commit has been rebased secondly.

@mhevery mhevery merged commit 2dc7e5c into angular:master Feb 8, 2018
@gdh1995 gdh1995 deleted the hook-toString branch February 8, 2018 06:10
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.

4 participants