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

refactor($parse): Create idents lazily #9229

Closed
wants to merge 1 commit into from

Conversation

lgalfaso
Copy link
Contributor

Create the ident functions lazily as these are not used for filters not object literal properties
The generated functions that are not used can cause issues with some browsers

Closes #9131

Note: This may have cause some performance improvements

@caitp
Copy link
Contributor

caitp commented Sep 23, 2014

were you actually able to reproduce the issues they were talking about?

@lgalfaso
Copy link
Contributor Author

@caitp This is what I did:

// parse.js@934

var evaledFnGetter = new Function('s', 'l', code); // s=scope, l=locals

We were generating a function where

code === 'if(s == null) return undefined;
s=((l&&l.hasOwnProperty("in"))?l:s).in;
return s;'
  • Created this patch, did the same and we were not creating a function like this

(we were creating one for animate, but that is something different)

Create the ident functions lazily as these are not
used for filters not object literal properties

Closes angular#9131
@@ -426,7 +430,7 @@ Parser.prototype = {
primary = this.object();
} else {
var token = this.expect();
primary = token.fn;
primary = token.fn || token.lazyFn && token.lazyFn();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this line change to

primary = CONSTANTS[token.text] || getterFn(token.text, this.options, this.text);

...and avoid the lazyFn + fn setup above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a primary can be an identifier or a constant, so it would look like something like this

primary = token.fn || token.identifier && (CONSTANTS[token.text] || getterFn(token.text, this.options, token.expression));

where expression and identifier need to be added

Copy link
Collaborator

Choose a reason for hiding this comment

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

The expression text already exists (this.text in Parser), I guess the identifier flag and token.fn || would be needed though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, we already have expression

@lgalfaso
Copy link
Contributor Author

lgalfaso commented Oct 6, 2014

closing this in favor of #9431

@lgalfaso lgalfaso closed this Oct 6, 2014
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.

Potential usage of reserved keywords in Parser.getterFn
4 participants