getConfig

[davidpfahler]

EDIT: Method name changed to getConfig.

Don't be confused by the branch name, method is now called getStateConfig as mentioned by @timkindberg.

Not sure how or where to do docs.

Fixes #294.

[timkindberg]

This looks good to me and I think will help many people. For docs, after this PR was merged, whoever merges it should create a new issue called "Docs for #295" and assign to me.

[nateabele]

The implementation seems fine, but I am hugely not a fan of overly verbose method names. I really think get() is fine.

[timkindberg]

@nateabele That works for me too.

[davidpfahler]

To demonstrate what the different suggestions would look like:
get

var url = $state.get('foo').url;

getStateConfig

var url = $state.getStateConfig('foo').url;

In my opinion, a method name should always hint at the parameters it is expecting and what it is returning. That is what I favor a slightly longer method name over just get, because what is $state.get supposed to return or take as a parameter.

That being said, I'm of course going to follow the project's teams' consensus.

[boennemann]

Isn't the State in the method name redundant?
I think getConfig would be just fine.

var url = $state.getConfig('foo').url;

We could then return the config of the current state by default, and use stateOrName to allow further specification of the desired state.

[timkindberg]

I like getConfig() with optional parameter.

[nateabele]

We could then return the config of the current state by default

This is redundant, given $state.current. The method should require a parameter. I dislike having n ways of doing things, especially simple things.

[boennemann]

It's just a convenience thing. I mean of course it's redundant*, but maybe it's not a bad idea to have an intuitive fallback.

*It's not even the same thing because getConfig returns a copy, rather than the actual object.

[davidpfahler]

I have to disagree with @nateabele for two reasons. First, it would be a very confusing API if we told developers they could read config for any state using getConfig, but $state.getConfig() wouldn't work. Instead you needed to use $state.current which is semantically of completely different pair of shoes.

Second, it's not just semantically different, but also a different object in memory. I could be mistaken, but by looking at the code it seems to me that $state.current is a reference to the config object, not a copy of it.

All in all, I'm strongly in favor of having a unified, easily understandable API.

[nateabele]

One person's intuitive fallback hides another person's bug. :-)

[nateabele]

Instead you needed to use $state.current which is semantically of completely different pair of shoes.

What's preventing you from passing the name of the current state? If you actually want the current state, you'd only ever use $state.current, not getConfig(). If you want to look up a state by name, you'd only ever use a method, not $state.current.

I'm not sure why this idea is so confusing.

[davidpfahler]

I understand your point, @nateabele, but I just happen to have the contrary opinion. The code is tested and provides a copy of the config object. This is the purpose of the method. $state.current is the actual configuration in action which you could change (not what we want). Hence, I would recommend to other devs to always use getConfig.

How do you handle disagreements on this project?

[nateabele]

How do you handle disagreements on this project?

Generally, it's been informal consensus among the main developers/contributors, which so far hasn't been terribly hard to reach. One of the main developers recently had a kid (and so has been pretty unavailable), and his style is to favor explicitness even more than I normally would, which is what I'm representing here.

Hence, I would recommend to other devs to always use getConfig.

Okay, great. Recommend that they always pass it a parameter.

$state.current is the actual configuration in action which you could change (not what we want)

That's actually not ever what we want, so maybe the correct approach for the sake of consistency is to ensure that $state.current is always a copy as well.

[timkindberg]

so maybe the correct approach for the sake of consistency is to ensure that $state.current is always a copy as well

Yeah I'm quite suprised to learn that $state.current is not a copy.

@davidpfahler don't worry about the arguing, we do it over everything :). You should mainly know that your contribution is much appreciated and thank you for helping.

For now let's just do getConfig with mandatory parameter. Its a good compromise.

[nateabele]

@davidpfahler Upon further reflection, here's why what you're suggesting is not line with the goals of the project. Basically, $state is currently straddling the line between being a singleton service that represents the entire state machine, and an object that represents the current state that the state machine is in. While the API was designed this way for the sake of expediency, the conceptual boundaries are hard to maintain, and having a method that acts as both a "class" method and "instance" method simultaneously depending on how it's called is just not helping the situation. Hopefully now you can understand, and we can move on.

[davidpfahler]

@nateabele That makes more sense to me now.

I think this was a healthy discussion showing some general problems in the API (like $state.current not being a copy). We should collect these general issues somewhere to discuss for planning a roadmap or something.

[ksperling]

I'm a bit late in the discussion, but what's the use case for returning copies of states? $state.current is not a copy on purpose, so that you compare it by identity to the original object you passed in, e.g. if ($state.current === MyAboutState)

Personally I think it would be fine to have $state.get('foo') return the real state and simply document that state configuration may not be changed at runtime. Angular.copy is a reasonably expensive operation, so making a copy purely to avoid "tempting" people into doing something stupid that's documented to not work seems a bit over the top.

[nateabele]

Okay fair enough. :-) I have some other work to do here tomorrow anyway, so I'll patch it then. @timkindberg Can you update the documentation accordingly?

[timkindberg]

Yup just make an issue and assign to me if u dont mind.

[timkindberg]

Oh fuck it. I'll just do it now.

[timkindberg]

K done. Only in API.