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

feat($parse): toggle evaluation of private fields via $parseProvider.allowPrivateFields() #4849

Closed
wants to merge 1 commit into from

Conversation

boneskull
Copy link
Contributor

Disallowing evaluation of private fields in Angular expressions should be toggleable through a config() block. Usage is shown in ngdoc comments.

It's worth noting that this behavior defaults to FALSE.

That is to say, if you want pre-Angular-1.2 behavior, you must do:

myModule.config(function($parseProvider) {
  $parseProvider.allowPrivateFields(true);
});

Please let me know if the unit tests are insufficient; they do not consider multiple "formats" of private variables as the tests do in 3d6a89e, but the main intent here was to show that the toggle works.

…allowPrivateFields()

Disallowing evaluation of private fields in Angular expressions should be toggleable through a `config()` block.  Usage is shown in ngdoc comments.
@Mparaiso
Copy link

Mparaiso commented Nov 9, 2013

I'd definetly vote for that , though OFF by default would make more sense, let me write javascript with MY dev team conventions.

@kofalt
Copy link

kofalt commented Nov 9, 2013

Looks like a productive alternative to the discussion in #4509 👍

I strongly agree with @Mparaiso; I'd really rather only enable string blacklisting when explicitly requested.
But as long as there's a way to disable it, I'm happier :)

@eknkc
Copy link

eknkc commented Nov 9, 2013

How about making the check itself customizable? Rather than a boolean flag for allow/disallow, it could accept a function that checks if a name is accessible or not and returns a boolean.

@boneskull
Copy link
Contributor Author

It defaults to false because I don't feel it's really my call to disable functionality by default. I'm more than happy letting the core devs paw through that hornet's nest. 😄

@eknkc May be a good idea but the motivation here was simply to toggle the existing functionality, not provide anything new.

@kofalt
Copy link

kofalt commented Nov 9, 2013

@boneskull sage :)

@eknkc Passing function(val) { return false } to disable a feature feels weird to me. I don't hate the idea of custom functions, but you can also check if the parameter's a boolean and toggle accordingly.

@lezhangxyz
Copy link

This is simple, to the point, and follows the existing $parseProvider conventions. 👍

@jonathannorris
Copy link

I agree that that this is a good solution, but also believe the default should be true.

Causing a wide reaching breaking change like this should only be done where absolutely necessary, I don't believe the proposed problem the breaking change solves warrant's the pain it will cause. Currently any teams using a MongoDB or CouchDB backed application will get a very rude introduction to 1.2.0.

@cburgdorf
Copy link
Contributor

I think it should be

myModule.config(function($parseProvider) {
  //set it to an empty string to completely ignore it.
  $parseProvider.privateFieldPrefix('_');
});

@petebacondarwin
Copy link
Contributor

+1 @cburgdorf - this is the ideal solution

@mlandalv
Copy link

mlandalv commented Nov 9, 2013

IMO it should be a combination of the OP and @cburgdorf.

It feels a bit magic to disable private fields because the prefix is an empty string. It feels like a not-so-obvious side effect. But on the other hand it's a nice feature to be able to change the prefix.

myModule.config(function ($parseProvider) {
    $parseProvider.privateFields(true);
    $parseProvider.privateFieldPrefix('_');
});

Notice I changed allowPrivateFields to privateFields. Isn't that more consistent with e.g. $locationProvider.html5Mode?

@alexgorbatchev
Copy link

👍 If this is made customizable without ability to just switch it off, then people will just set random strings as prefixes and it will just slow their applications down.

@petebacondarwin
Copy link
Contributor

I think the api should be something like:

$parseProvider.restrictFieldsMatching(/^_/);
On 9 Nov 2013 16:59, "Martin Landälv" notifications@github.com wrote:

IMO it should be a combination of the OP and @cburgdorfhttps://github.com/cburgdorf
.

It feels a bit magic to disable private fields because the prefix is an
empty string. It feels like a not-so-obvious side effect. But on the other
hand it's a nice feature to be able to change the prefix.

myModule.config(function ($parseProvider) {
$parseProvider.privateFields(true);
$parseProvider.privateFieldPrefix('_');
});

Notice I changed allowPrivateFields to privateFields. Isn't that more
consistent with e.g. $locationProvider.html5Mode?


Reply to this email directly or view it on GitHubhttps://github.com//pull/4849#issuecomment-28131421
.

@petebacondarwin
Copy link
Contributor

With null meaning "switch it off"
On 9 Nov 2013 17:33, "Pete Bacon Darwin" pete@bacondarwin.com wrote:

I think the api should be something like:

$parseProvider.restrictFieldsMatching(/^_/);
On 9 Nov 2013 16:59, "Martin Landälv" notifications@github.com wrote:

IMO it should be a combination of the OP and @cburgdorfhttps://github.com/cburgdorf
.

It feels a bit magic to disable private fields because the prefix is an
empty string. It feels like a not-so-obvious side effect. But on the other
hand it's a nice feature to be able to change the prefix.

myModule.config(function ($parseProvider) {
$parseProvider.privateFields(true);
$parseProvider.privateFieldPrefix('_');
});

Notice I changed allowPrivateFields to privateFields. Isn't that more
consistent with e.g. $locationProvider.html5Mode?


Reply to this email directly or view it on GitHubhttps://github.com//pull/4849#issuecomment-28131421
.

@lrlopez
Copy link
Contributor

lrlopez commented Nov 9, 2013

Guys, take a look into #4859 and tell me what you think... It implements Pete's proposal.

@gabrieledarrigo
Copy link

+1 for this

@TheHippo
Copy link

Thats what we need.

@ghost ghost assigned chirayuk Nov 11, 2013
@wedgybo
Copy link

wedgybo commented Nov 12, 2013

👍

1 similar comment
@geddski
Copy link
Contributor

geddski commented Nov 13, 2013

+1

@vojtajina vojtajina closed this in 4ab16aa Nov 14, 2013
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.