Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Index refactor - RFC #1474

Merged
merged 7 commits into from
Nov 28, 2014
Merged

Index refactor - RFC #1474

merged 7 commits into from
Nov 28, 2014

Conversation

evs-chris
Copy link
Contributor

There are a bunch of edge-cases with index resolution that have popped up in the last few weeks. While trying to find solutions to the issues, it started to look like a refactor may be in order, so this is my attempt.

Don't try to propagate index changes on rebind

Tracking index refs that may or may not be object keys through rebind is challenging. This disposes with that in exchange for resolvers that register with their target ancestor fragment to receive updates. In createReferenceResovler, index references are searched for up-tree. This may not be as performant as having the references provided upon creation, especially on very deep (tall?) trees, but it doesn't look like it would there is a significant penalty.

In short, sections are the new single source of truth for index references.

This address the issue in #1457, which is already fixed in #1460, but this feels a little cleaner to me.

{{#outerList}}
  {{#innerList}}
    {{someRef}}
  {{/}}
{{/}}

Here, shuffling in outerList doesn't step on innerList because the indices aren't pushed too far down the tree during rebind.

Question should outer index refs be visible in isolated components. My gut says no.

Keep track of what sort of reference was resolved in resolution

Reference resolutions add an indicator to the resolution to keep track of what sort of resolution it was. This fixes #1462 by skipping numeric checks on key references.

In this example:

{{#obj:k}}{{@key}} {{k}}{{/}}
{ obj: { '01': 'not a number', '000001': 'also not a number', '01189998819991197253': 'emergency services' } }

you get what you'd expect because the keys resolve to '@k01', '@k000001', '@k01189998819991197253' where the 'k' (as opposed to 'i') avoid the number test altogether in decodeKeypath.

Object iteration has first-class support

Object keys are tracked in addition to the index instead of in place of it. This addresses #1444 by binding the fragment ordinal to index references instead of having both key and index refs resolve to the index. It also handles switching an each section between object and array(-like).

{{#obj:k,i}}<p>{{k}} {{i}} {{.}}</p>{{/}}
{ obj: { foo: 1, bar: 2 } }

Result:

<p>foo 0 1</p><p>bar 1 2</p>

@index and @key work the same way.

moves index propagation out of rebind
ref resolver creation searches up-tree for indices
resolvers register with their target fragments to receive updates
makes object key interation first-class instead of overriding the index
resoltions carry a type indicator to avoid turning number-like keys into
  numbers
object iterations can access both the key and ordinal index
index refs in components no longer get added to the viewmodel
iterations can now switch between array and object
@martypdx
Copy link
Contributor

Question should outer index refs be visible in isolated components. My gut says no.

Isolated only sees what is passed into component parameters and doesn't get or set implicitly into parent view, so an outer index would only be accessible if passed as parameter, which AFAICT mappings take care of.

@evs-chris
Copy link
Contributor Author

Thanks. Fixed and test-cased.

@evs-chris
Copy link
Contributor Author

I forgot to mention that this is a breaking change. @index and @key are no longer interchangeable. The template format has changed slightly as i nodes are now an array of objects instead of a string.

If this gets ok enough to be merged at some point, the template version number should probably be bumped.

@martypdx
Copy link
Contributor

Hey @evs-chris -

Does this make it possible to "get" special refs? The current mapping treats specials refs differently and caches a local copy that the resolver updates when it changes. I know my question is a bit fuzzy, I don't fully have my head around special refs, but take a look at current code.

Basically is there a resolved keypath or something that can be used to get the value more like "normal" refs?

@evs-chris
Copy link
Contributor Author

Explicitly mapped special refs are available to get. In this anything else exists entirely outside of the viewmodel. This is sort of a fuzzy topic anyways, since special refs, being entirely dependent up view, seem to want to exist entirely outside of the viewmodel. It should be possible to make them implicitly available to get in the same way that they are in edge. Of course @ special refs are not available to get in this or edge, since the viewmodel takes @ keypaths and decodes them immediately.

{{#each list:i}}
  <cmp i="{{i}}" idx="{{@index}}">My view and viewmodel knows i and idx</cmp>
  <cmp>My viewmodel does not know i and idx, but my view does</cmp>
{{/each}}

Having view-dependent values, which may be shadowed, accessible in the viewmodel tends to get a little bit fuzzy. They are context specific, but they don't really map to a particular keypath at this point (here or edge).

{{#each list:i}}
  <cmp />
{{/each}}
Ractive.components.cmp = Ractive.extend({
  template: '{{#each foo:i}}<span on-click="foo()">{{i}}</span>{{/each}}',
  data: { list: [1, 2, 3] },
  foo: function() { console.log(this.get('i')); }
});

In this example (in edge), the outer index gets logged to the console, not the inner one.

Are you looking for something like ractive.get('foo.bars.@index') or only named indices? I considered having sections set their indices in the viewmodel prefixed with their keypath and some special sigil, but then it occurred to me that it couldn't be done sanely if there is more than one section that iterates a keypath.

@evs-chris
Copy link
Contributor Author

After sleeping on it, it occurs to me that the @key and template format breaking changes aren't necessary. They are now worked around.

@martypdx
Copy link
Contributor

Are you looking for something like ractive.get('foo.bars.@index') or only named indices? I considered having sections set their indices in the viewmodel prefixed with their keypath and some special sigil, but then it occurred to me that it couldn't be done sanely if there is more than one section that iterates a keypath.

The issue is that in mappings there end up being two classes of references: Those that can be retrieved on demand by keypath from the owner viewmodel, and those that are copied to the component as the value changes. References and Keypath expressions resolved to keypaths so they're in the first category. Expressions and Special References are in the later category. I don't think there's anyway around it, so it's fine. Thx

Is this ready to merge?

@evs-chris
Copy link
Contributor Author

Is this ready to merge?

If you agree that the approach of having fragments manage their dependents directly instead of trying to push index/key updates with rebind, then this is good to go.

If you want to punt on this until your component mappings work is merged, I'd be happy to handle merge conflicts on this side.

@martypdx
Copy link
Contributor

If you want to punt on this until your component mappings work is merged, I'd be happy to handle merge conflicts on this side.

Probably the way to go. I just have to finish the "undo" reverse mappings when computed props take ownership, then the component goes away...

@martypdx
Copy link
Contributor

@evs-chris mappings PR now merged

Conflicts:
	src/shared/resolveRef.js
	src/virtualdom/items/Partial/_Partial.js
evs-chris added a commit that referenced this pull request Nov 28, 2014
Index refactor - RFC

Fixes #1476, Fixes #1462, Fixes #1444
@evs-chris evs-chris merged commit f175879 into ractivejs:dev Nov 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong #each index when object key
2 participants