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

feat($parse): config option blockPrivateSymbols=false #4926

Closed
wants to merge 1 commit into from

Conversation

chirayuk
Copy link
Contributor

No description provided.

if (typeof name !== 'string' && toString.apply(name) !== "[object String]") {
return name;
}
function ensureSafeMemberName(name, fullExpression, options, allowConstructor) {
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 pass just the "blockPrivateSymbols", it feels weird, that this function accepts "options" and "allowConstructo" (which is an option too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@vojtajina
Copy link
Contributor

LGTM.

I was not sure about the "blockPrivateSymbols" config name, but can't think of any better. I was also thinking about making the "private pattern" configurable, so that people can use different convention than _, but that's probably not necesary at all, as this is mostly for people using closure compiler, which implies _.

@chirayuk
Copy link
Contributor Author

I agree that making it configurable has a little benefit.  Based on what is picked, it could even block Angular core's own expressions which won't have an idea of the configured value.

@rodyhaddad
Copy link
Contributor

It's good to note that this PR has the blocking disabled by default 👍

@jr314159
Copy link

👍

@dgieselaar
Copy link

Thank you!

@goya
Copy link

goya commented Nov 13, 2013

thanks for the code bloat.

@petebacondarwin
Copy link
Contributor

@goya - Hi from the look of your GH repositories, it seems you are quite keen on mobile apps. So I understand your concern about keeping the code base tight. That being said after this patch the minified gzipped angular.js file is still only 36435 bytes, with the patch adding only about 0.14% to the size of the file.

@vojtajina vojtajina closed this in 4ab16aa Nov 14, 2013
@vojtajina
Copy link
Contributor

@goya thanks for your useful comments and contribution to this project.

@alexgorbatchev
Copy link

Thank you! 👍

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
Hiding `_*` properties was a feature primarily for developers using Closure compiler and Google JS
style. We didn't realize how many people will be affected by this change.

We might introduce this feature in the future, probably under a config option, but it needs more
research and so I'm reverting the change for now.

This reverts commit 3d6a89e.

Closes angular#4926
Closes angular#4842
Closes angular#4865
Closes angular#4859
Closes angular#4849

Conflicts:
	src/ng/parse.js
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
Hiding `_*` properties was a feature primarily for developers using Closure compiler and Google JS
style. We didn't realize how many people will be affected by this change.

We might introduce this feature in the future, probably under a config option, but it needs more
research and so I'm reverting the change for now.

This reverts commit 3d6a89e.

Closes angular#4926
Closes angular#4842
Closes angular#4865
Closes angular#4859
Closes angular#4849

Conflicts:
	src/ng/parse.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants