RFC: Huge Refactor + Feature Additions #286
Comments
|
I was actually just poking around your fork last night and already reviewed all of your commits. I was really happy with what I saw and super fucking excited to see so much progress! I do need to review it all again, now that I know you want actual feedback. Comments forthcoming... |
|
The usage of I realize that this means your change to the URL-relative addressing no longer matches semantically but I'd suggest not fighting years of intuition concerning filesystems and use |
|
Mostly I'm just super happy about all of it. Your refactoring has helped immensely with my understanding of the code, especially the stateBuilder object. I do have to second the concern by @eahutchins on the
Let me go off on a tangent, just to get some ideas out. One solution, which would require some breaking changes would be to use something else in state declarations. Maybe a pipe. So instead of: Ideally, we should have a common use for any characters used in ui-router:
Some other ideas for child name separators:
Finally, it would also be ideal to only have to use Hey, or we could just use the god damn |
|
@timkindberg: what do you object to about a leading |
|
What is the problem with using urls as a reference? People will not have to I think it would be best to use common knowledge convention such as : /root Etc
|
|
@eahutchins yeah I don't really object, I think we are the same page. I was just throwing some ideas out. @ProLoser, the reason we didn't want to use url syntax is because we are afraid it will create confusion, mixing the idea of states with urls, when they are different concepts. It's still a good option though, but it just carries that weight with it. @ProLoser what do you mean by:
|
|
@eahutchins when I said that I think |
|
I think you are creating confusion by making up a completely arbitrary new syntax out of thin air. This syntax is used for folder traversal, and unless the paradigm doesn't match 1-1 I don't think it's a problem. |
|
Ok let's just play it out then. If we used // configuration
$stateProvider
.state("parent", {
templateUrl: 'parent.html',
url: "/parent"
})
.state("parent/child", {
templateUrl: 'child.html',
url: "&/child", // use '&' to append to parent state's url
});
// navigation
$state.transitionTo("parent/child", { param1: 'hello' });
// absolute addressing
$state.go("/parent/child");
// relative addressing
$state.go("./sibling");
$state.go("../parent");
$state.go("../../../ancestor");
// util methods
$state.includes("parent/child");
$state.is("parent/child");
$state.href("parent/child", { param1: 'hello' });// directives
<a ui-sref="parent/child({ param1: 'hello' })">Link</a>I think this is making a lot of sense to me... what do you guys think? |
|
Now I'm confused. You are using & in a url option?
|
@nateabele -- I want to find time to look at your work. I'm under a deadline and not getting much sleep but hopefully soon. Thanks for keeping me in the loop! |
|
No me gusta. It confuses state names with URLs, between which there should be strict conceptual boundaries. Kinda ambivalent about the other bits as well. We should be able to have a single, consistent character to represent inheritance & upward traversal. |
|
@ProLoser I kind of snuck that '&' in there. It was an idea I had a while back, which actually most were in favor of. Using '&' to explicitly declare appended urls instead of doing it automatically for the user. There were lots of users who didn't realize the appending was happening. The '&' is similar to appending in LESS. Its actually already on the roadmap: #181. @stu-salsbury I numbered your questions.
@nateabele of course you are ambivalent. You probably just wanted to merge your hard work instead of having us bitch about it. I don't blame you.
Considering @eahutchins, @ProLoser and I all feel like the |
|
Whatever, I put in my 2c but I'm not married to it. I will let you guys
|
No, feedback is good, and I appreciate it. I have more work to do on this stuff anyway. I just don't want to go down the rabbit hole of single-character syntax deliberations. The most important things are consistency and overall conceptual integrity of the library. I've participated in more than enough of these discussions to know that trying to speculate about what will or won't confuse people is not useful. Maintain conceptual integrity, and follow a simple set of well-documented rules. The rest will take care of itself. Anyway, that's enough preaching from me. I honestly don't care what the specific character is (I actually had it as |
This has already been discussed to death in other contexts. :-) One of the explicit goals of the project is to use it completely without URLs. There are explicit conceptual and abstraction-layer boundaries between the two, and changing to URL-style state selectors muddies the water in a really bad way. |
|
I think the answer is colons. :) Edit: I'm only partially joking. |
|
Just to add a reference to where the I'm personally in favor for the |
|
Hey @jeme, thanks for the reference. Sorry I forgot to cc you. :-) |
|
After thinking about the Ok moving on... I re-read @eahutchins early post again and it actually sums up how I am feeling. First he says:
But I wanted to flesh it out some more and see it in action. So let's write it out (you might be able to tell that I like to see this stuff visually and I hope it helps you guys too). If we assume that // -- ABSOLUTE -- //
// No prepended special character
$state.go('full.state.name');
// -- CHILD -- //
// '.' takes us down, but we also have to say which child
$state.go('.child');
// -- GRANDCHILD -- //
// Same as child but repeat '.name' as needed
$state.go('.child.grandchild');
// -- SIBLING -- //
// '~' takes us lateral, but we also have to say which sibling
$state.go('~sibling');
// -- SIBLING'S CHILD -- //
$state.go('~sibling.child');
// -- PARENT -- //
// '^' takes us up. No need to specify which parent, there's only one.
$state.go('^');
// -- PARENT'S SIBLING -- //
// '^' takes us up. Need to specify the state if its not going to be the parent.
$state.go('^uncle');
// -- GRANDPARENT -- //
// Same as parent, can use as many '^' as needed
$state.go('^^');
// -- GRANDPARENT'S SIBLING -- //
$state.go('^^greatuncle');It's easy to understand, doesn't use |
|
I dig that, barring the |
|
Hmm I disagree. That looks like up, down, up, down to me. Also I don't have a |
|
So I came up with the specs on the referencing system I am proposing. Its fairly technical but it shows the usage rules. And would help with the parsing logic. Symbols: Chunks: Usage Symbols: Usage Rules: |
|
If you think of these things as operators I think it will be a lot less confusing: composition: ascent: I really don't like note: I do think being able to ascend out of some |
|
@eahutchins can you list out the various uses of your proposal, that would help me. Just copy and modify my list from above comment. Because I didn't intend to use a |
|
@nateabele, I don't think we need uncle support. The costs seem to outweigh the gains. If you drop it you can go all the way back to just supporting '^' and '.' ^ goes up, and ~ goes lateral and . goes down and ^ does not support symbols after the ^^^^s What kind of real life circumstances require "uncles". Bailing you out of jail, going to a ballgame. Neither requires going to his house. Seriously, though, on the rare case where you need to visit your uncle you can you use a naked (i.e. absolute) state path? I figure it's most likely to show up when you're building a tree in the UI, so using absolute state paths would make some sense... |
Just wanted to note that Also I did not use a @stu-salsbury
That's fine, but the fact stands that my proposal is pretty elegant while achieving all possible use cases. If we don't want to include uncle targeting (or other types of targeting) because its too much extra logic behind the scenes, then that's fine. However, my syntax and the rules for referencing are solid so if we ever wanted to add the functionality in, I'll just be recommending and referring to the same spec. |
But that doesn't make sense in context. Compare to your example of |
|
@nateabele Is that how your current syntax works? |
|
@timkindberg Basically, yes. Don't get me wrong. the regular expression that parses the path syntax is easy enough to change (it's already gone through a few different syntax iterations), i just think the double |
|
@nateabele okay so your proposal would be like this?
// -- ABSOLUTE -- //
$state.go('full.state.name');
// -- CHILD -- //
$state.go('^.child');
// -- GRANDCHILD -- //
$state.go('^.child.grandchild');
// -- SIBLING -- //
$state.go('^sibling');
// -- SIBLING'S CHILD -- //
$state.go('^sibling.child');
// -- PARENT -- //
$state.go('^');
// -- PARENT'S SIBLING -- //
$state.go('^.^.uncle');
// -- GRANDPARENT -- //
$state.go('^.^');
// -- GRANDPARENT'S SIBLING -- //
$state.go('^.^.^.greatuncle');The only thing that makes no sense is when |
I remember @ksperling being in favor of that syntax... where the hell is he anyway? Probably changing diapers. |
Yeah, he was also in favor of |
|
I'm working on a commercial product so discussing specific use cases is a bit problematic. I'll put a brief note where and why I differ below (but I can live with what you're proposing): // -- ABSOLUTE -- //
// No prepended special character
$state.go('full.state.name');
// -- CHILD -- //
// '.' takes us down, but we also have to say which child
$state.go('.child');
// -- GRANDCHILD -- //
// Same as child but repeat '.name' as needed
$state.go('.child.grandchild'); agreed. // -- SIBLING -- //
// '~' takes us lateral, but we also have to say which sibling
$state.go('~sibling');
// -- SIBLING'S CHILD -- //
$state.go('~sibling.child'); In my syntax // -- SIBLING -- //
// `^` takes us up, then `.` down to sibling
$state.go('^.sibling');
// -- SIBLING'S CHILD -- //
$state.go('^.sibling.child'); // -- PARENT -- //
// '^' takes us up. No need to specify which parent, there's only one.
$state.go('^');
// -- GRANDPARENT -- //
// Same as parent, can use as many '^' as needed
$state.go('^^');agreed. // -- PARENT'S SIBLING -- //
// '^' takes us up. Need to specify the state if its not going to be the parent.
$state.go('^uncle');
// -- GRANDPARENT'S SIBLING -- //
$state.go('^^greatuncle');Here I think the // -- PARENT'S SIBLING, AKA GRANPA'S CHILD -- //
// '^' takes us up twice, `.` down to uncle
$state.go('^^.uncle');
// -- GRANDPARENT'S SIBLING -- //
$state.go('^^^.greatuncle');and you can do: // -- STATIC RELATIVE -- //
// up from a static path to a relative path
$state.go(myPath + '^.checkout');The grammar is something like: I also think whatever gets picked should make the most sense in the bigger context (i.e. require the least new learning to understand). |
|
@eahutchins I'm in complete agreement with your proposal. @nateabele what do you think of it? |
|
There are a few weird corner-cases in that grammar, In the future we might want other operators like |
|
I gotta side with @nateabele here on the 1. How I would understand it
So conceptually it becomes (pseudo code): Might be because how I think about these things. In other words:
2. ImplementationSecond reason is that it may be simpler to implement. Illustration |
|
@atian25 please start a new discussion. This one is solely for reviewing @nateabele changes as listed in his original post. |
I hear that and I get that, and I got it when @nateabele was explaining it, and honestly I'm fine if that's how it needs to be. Its just simply that I like how it looks without the So anyway I think we are to a point where we all more or less agree. Here are the final use cases per all discussions:
// -- ABSOLUTE -- //
$state.go('full.state.name');
// -- CHILD -- //
$state.go('.child');
// -- GRANDCHILD -- //
$state.go('.child.grandchild');
// -- SIBLING -- //
$state.go('^.sibling');
// -- SIBLING'S CHILD -- //
$state.go('^.sibling.child');
// -- PARENT -- //
$state.go('^');
// -- PARENT'S SIBLING -- //
$state.go('^.^.uncle');
// -- GRANDPARENT -- //
$state.go('^.^');
// -- GRANDPARENT'S SIBLING -- //
$state.go('^.^.^.greatuncle');Can we all agree on this? |
|
I'm fine with it, it's basically viewing I think this grammar is a bit more complicated too: |
|
Actually @jeme your last example is an absolute state, selecting the green |
|
whoa I totally did not see @jeme 's graphic before... |
|
@jeme great graphic! will def be using that in the guide!!!
Its hard to tell what you were in favor of here. Is this for or against keeping |
|
I'd prefer |
|
@eahutchins ok cool, me too. Ok this is the FINAL spec. Leaving some room for @nateabele to implement however is easiest.
// -- ABSOLUTE -- //
$state.go('full.state.name');
// -- CHILD -- //
$state.go('.child');
// -- GRANDCHILD -- //
$state.go('.child.grandchild');
// -- SIBLING -- //
$state.go('^.sibling');
// -- SIBLING'S CHILD -- //
$state.go('^.sibling.child');
// -- PARENT -- //
$state.go('^');
// -- PARENT'S SIBLING -- //
$state.go('^^.uncle');
// if possible, otherwise
$state.go('^.^.uncle');
// -- GRANDPARENT -- //
$state.go('^^');
// if possible, otherwise
$state.go('^.^');
// -- GRANDPARENT'S SIBLING -- //
$state.go('^^^.greatuncle');
// if possible, otherwise
$state.go('^.^.^.greatuncle');NO MORE DISCUSSION ON THIS IF AT ALL POSSIBLE! I'm channeling @nateabele right now. I'm sure he's sick of this circus (however necessary). |
|
This would largely be my approach on the implementation: function lookup(path) {
var sp = path.split('.');
var sc = current.split('.'); //the "full name" of the current state.
function select(selector) {
switch (selector) {
case "":
sc = [];
break;
case "^":
sc.pop();
break;
default:
sc.push(selector);
break;
}
}
for (var i = 0; i < sp.length; i++)
select(sp[i]);
return sc.join('.'); //the targeted state name
}and in order to treat function modify(path) {
switch (path.charAt(0)) {
case '.':
return path.substring(1);
case '^':
return path;
}
return '.' + path;
}A Demonstrating fiddle on the output. It lacks Error handling as it is there, but I didn't know if you had an final implementation already @nateabele... if not, well your welcome to grap it... (it only returns the targeted name) |
|
Ohh... and ill correct the diagram according to what @nateabele comes up with if you wan't to use it @timkindberg, but please download it and put it somewhere else to host (github or something), as I won't guaranty that it stays in my dropbox >.< |
|
I have just committed and merged the final implementation for this. The tests show all the various targeting permutations. As you can see, relative targets must always be dot-separated, because (a) the parser code is just simpler, and (b) having multiple ways of doing such low-level syntax things is generally regarded as A Bad Idea™. Having said that, I do want to thank everyone for their input, as the final implementation is clearly more robust than my initial one. @jeme thanks for putting together that awesome diagram, it will be of great use in the docs for this. @timkindberg you may notice that not all of your permutations are covered directly in the tests (great uncles, and so on). I didn't want to add a bunch more states to the test, but those are still covered by the syntax. And @eahutchins, bringing the BNF! Remind me never to oppose you in a CS debate. :-) |
|
💯 This is so cool @nateabele! I think I can thank you from all of us for taking the time (that you could have been doing other things) to make some much needed and incredibly valuable changes to ui-router. Bravo dude! |
|
Bah... ill comment on the new issue instead. |
Brace yourselves. This is pretty freakin' long.
I Have a Branch to Which I Have Committed A Whole Bunch of Crap
For reference, see here. We'll be talking about what's going on in those last few commits (the ones with my name on them). Let's just go straight down the list, shall we?
State-Building Refactor
The
registerState()method, which is used to... register new states, was previously pretty large. I have refactored it into a state builder object, which makes the various aspects of constructing a state much more comprehensible and easy to extend (albeit only internally for now... maybe we can leverage this in the provider API to allow people to generate or auto-assign their own state attributes... I literally thought of that just now). It also makesregisterState()around 20 lines of code, which is nice.The ^ Character
We have a new magical character:
^. This character is basically used to append or inherit from the current state when transitioning. You can check out the tests which demonstrate this functionality using the new$state.go()short-hand method. You can transition to a^childstate, a^.siblingstate, or a^.^.parentstate. This syntax is implemented directly infindState(), so it should work for anything that accepts a state name, however, any calls tofindState()must be refactored to pass the state that the lookup is relative to. I also plan to make the^character for inheriting parent URLs, i.e.url:^/child`, because the current functionality of appending to the parent URL automatically is confusing and inflexible.Also, you may notice that those tests demonstrate...
Cascading Parameter Inheritance
Yup, that's a thing now. It happens automatically when using
$state.go(), or when passing{ inherit: true }to$state.transitionTo()(and yup, that's been refactored to accept an options hash as its third parameter).The
$viewserviceThe template-management code has been refactored into the new
$viewservice. This code is still pretty crap, and needs some work, but at least it's not directly embedded inside$stateanymore. I also still need to write some tests for it, so there's that, too.View Targeting
I'm kind of lying by including this in the list, because it's actually not done (yet), but will be committed soon. In short, the current approach to view targeting is crap. It is the single most confusing aspect of what is an otherwise fairly straightforward library. We've gotten more questions on that, than on any other single aspect of this library (except maybe for resolves, but those fixes are already in progress).
Here's the thing: if I'm targeting a view, I should be able to use a simple dot-syntax, i.e.
viewA.viewB.viewC, regardless of what state created that view. If a named view exists, then it exists. If it doesn't, it doesn't. Which state created it is totally irrelevant. This also makes view targeting much more flexible, as we can do things like allow relative targeting with^, similar to the above. This is going to get way more important as people try to use this library to scale out projects, or create redistributable 3rd-party parts of applications, which may be "mounted" at arbitrary parent states within an application.In Summary
I didn't want to submit a PR for this until we had a chance to discuss it, and because it's still in progress, but I wanted to collect everyone's thoughts on this as I wrap up these changes. Thanks for reading. /cc @ksperling, @timkindberg, @ProLoser, @stu-salsbury, @eahutchins
The text was updated successfully, but these errors were encountered: