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

Directive scopeTypes like react's propTypes #11657

Closed
kentcdodds opened this issue Apr 20, 2015 · 17 comments
Closed

Directive scopeTypes like react's propTypes #11657

kentcdodds opened this issue Apr 20, 2015 · 17 comments

Comments

@kentcdodds
Copy link
Member

One of my favorite parts of React is PropTypes. I would really really love to be able to utilize this kind of api in Angular.

For those not familiar, PropTypes essentially allows you to validate the api to your component so the developer is warned in the console when providing properties that are incorrect, of the wrong type, or are required and not specified.

I love this so much that I created apiCheck.js to bring this to the VanillaJS world. And I even started a little prototype (WIP) for adding scopeTypes to Angular 1.x directives.

I would happily work on adding this functionality to angular if there's enough interest. Even something as simple as a function that would be called to validate the scope properties would be nice... Just keeping validation out of my controller logic is what I'm looking for.

Note: I'm more interested in getting this concept accepted and implemented than I am in actually using apiCheck... I don't care what's used. I just want it to be easy.

@petebacondarwin
Copy link
Member

I like this idea but I am concerned about the performance implications and how it could actually be implemented. Each of the directive "bindings", i.e. the scope or bindToController properties, are updated independently triggered by various watches that are set up a linking time. So I would expect that this validation hook would need to be triggered for individual bindings as their watch handler is triggered.

What do others think?

@kentcdodds
Copy link
Member Author

Yeah, I'll give more input later (parked now to respond quickly). But I've
been using angular-scope-types to hear success for about two months out so.
Performance is not an issue because the checking logic is returned from a
function that is not called in production. So the price I pay is the bytes
which I believe is outweighed by the benefit of a defined api.

https://github.com/alianza-dev/angular-scope-types

  • Kent C. Dodds

(Sent from my mobile device, please forgive typos or brevity)
On Jul 15, 2015 7:57 AM, "Pete Bacon Darwin" notifications@github.com
wrote:

I like this idea but I am concerned about the performance implications and
how it could actually be implemented. Each of the directive "bindings",
i.e. the scope or bindToController properties, are updated independently
triggered by various watches that are set up a linking time. So I would
expect that this validation hook would need to be triggered for individual
bindings as their watch handler is triggered.

What do others think?


Reply to this email directly or view it on GitHub
#11657 (comment)
.

@kentcdodds
Copy link
Member Author

Also, the tests could probably be written a little more clearly so you
could glean the api by looking at the test. And docs are non-existent, but
the demo should reveal the basic idea pretty well:
https://github.com/alianza-dev/angular-scope-types/blob/master/demo/script.js

  • Kent C. Dodds

(Sent from my mobile device, please forgive typos or brevity)
On Jul 15, 2015 8:09 AM, "Kent C. Dodds" kent@doddsfamily.us wrote:

Yeah, I'll give more input later (parked now to respond quickly). But I've
been using angular-scope-types to hear success for about two months out so.
Performance is not an issue because the checking logic is returned from a
function that is not called in production. So the price I pay is the bytes
which I believe is outweighed by the benefit of a defined api.

https://github.com/alianza-dev/angular-scope-types

  • Kent C. Dodds

(Sent from my mobile device, please forgive typos or brevity)
On Jul 15, 2015 7:57 AM, "Pete Bacon Darwin" notifications@github.com
wrote:

I like this idea but I am concerned about the performance implications
and how it could actually be implemented. Each of the directive "bindings",
i.e. the scope or bindToController properties, are updated independently
triggered by various watches that are set up a linking time. So I would
expect that this validation hook would need to be triggered for individual
bindings as their watch handler is triggered.

What do others think?


Reply to this email directly or view it on GitHub
#11657 (comment)
.

@kentcdodds
Copy link
Member Author

I've updated the documentation to describe how to use my library. But if this were implemented directly into angular the API would be much better (wouldn't have to do most of the stuff... It's just the getScopeTypes function that we'd still have to do).

https://github.com/alianza-dev/angular-scope-types

Also updated the demo so you can play with it: https://jsbin.com/kuqeye/edit?html,js,output

@petebacondarwin
Copy link
Member

@kentcdodds if you could put together a very basic PR with a Proof of Concept for what the API would look like and how it would not impact production performance then I would be happy to discuss including it.

@kentcdodds
Copy link
Member Author

Fantastic! Would love to get something like this supported in core. Do you mind giving me a tip as to a good place to start? As well as where my tests should go?

@petebacondarwin
Copy link
Member

The unit tests go in the test folder :-) If you are modifying the $compile service, which I guess you would need to do, then you look inside the test/ng/$compileSpec.js file. Warning it is big.

kentcdodds pushed a commit to kentcdodds/angular.js that referenced this issue Sep 2, 2015
Add happy path tests and initial implementation for directive scopeTypes

angular#11657
@kentcdodds
Copy link
Member Author

@petebacondarwin, I've finally made my first commit for this feature. Looking for feedback. I'm certain this isn't exactly how you want it implemented, but it shows the general idea of what I'm going after and what I'd expect.

Also, I'm not sure why, but doing this breaks other (seemingly unrelated) tests. They fail with something like:

DUMP: 'LEAK', '$scope', '"$SCOPE"'

Not sure what's going on there, but if you ddescribe my tests, they pass fine.

Looking for feedback and direction so I can iterate on this.

@PatrickJS
Copy link
Member

+1 this is a great idea and is something that's needed in Angular 1

@PatrickJS
Copy link
Member

since we're getting a Schema (angular/angular#2014) feature in Angular 2 perhaps we can do something similar for Angular 1

@petebacondarwin
Copy link
Member

@kentcdodds I left some feedback on your commit.

@fabm22
Copy link

fabm22 commented Nov 19, 2015

Great effort !

@kentcdodds
Copy link
Member Author

Thanks @fabm22. I'd love to work on this more, but I don't think that I'll actually be able to make this happen. I hope it does get implemented eventually, but I don't think that I'll be the one developing it :-(

@cawabunga
Copy link

I take @kentcdodds idea and a bit rewrite it. cawabunga/angular.js@g3_v1_5...cawabunga:feature-directive-scope-validation
Disclaimer: i'm still working on it; commits are not formed by guide; needs more tests and docs update
When it be ready I will send a PR.

@kentcdodds
Copy link
Member Author

This is great to see!

@cawabunga
Copy link

I have few conceptual questions.
When checking should be called, only for initial values or when values change, and what about =-binding?
What to do with failed ones?
How it should be configured?
By now, the checking is calling every time the binding changes the value. Failed checking warn the logger. And now, there is no any configuration.
Thanks for your comments!

@kentcdodds
Copy link
Member Author

When checking should be called, only for initial values or when values change, and what about =-binding?

I would expect validation to happen every time a = binding is updated.

What to do with failed ones?

Just log a warning to the console.

How it should be configured?

Doesn't your implementation show how to configure it? You have some tests that seem reasonable to me ¯_(ツ)_/¯ Or are you talking about app-level configuration? I don't see any reason to have app-level config.

Full disclosure: I don't work in angular codebases anymore, so my opinions should not really be considered for much :-)

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

No branches or pull requests

5 participants