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

153 cross frame disposal with named templates #405

Merged
merged 8 commits into from Apr 10, 2012

Conversation

Projects
None yet
3 participants
@SteveSanderson
Copy link
Contributor

SteveSanderson commented Mar 23, 2012

No description provided.

@SteveSanderson

This comment has been minimized.

Copy link
Contributor

SteveSanderson commented Mar 23, 2012

This originated in the discussion at #153

@SteveSanderson

This comment has been minimized.

Copy link
Contributor

SteveSanderson commented Mar 23, 2012

To be honest, I'm not all that happy with adding such a big chunk of code (and extra parameters and caches) just to handle a very low-value scenario (cross-frame template rendering). We need to be pretty discerning about what goes in core to keep it maintainable and not ultimately get bloated.

Ryan/Michael - do you think we should really support this? If it's not all that valuable, we could just merge in a68262b and leave out the other commits. Then the simple case of anonymous templates continues to work, and if someone really wants named ones, they could write a custom template source.

@rniemeyer

This comment has been minimized.

Copy link
Member

rniemeyer commented Mar 23, 2012

My vote would be to skip it. Looks like there are several options that would already work like adding the template to the original document and using anonymous templates. Otherwise, creating a custom template source to handle it would still be an option.

@mbest

This comment has been minimized.

Copy link
Member

mbest commented Mar 23, 2012

@rniemeyer - A custom template source would not be able to handle it since it would run into the same problems we're having here, that the template engine needs to know what document to load the template from. This can only be solved in the core library. I'll look over what Steve has done and see if it can be made more compact.

@SteveSanderson

This comment has been minimized.

Copy link
Contributor

SteveSanderson commented Mar 23, 2012

We could keep the change that passes the doc info into the template
engine, but drop the changes to the template engine/source itself.

That way if someone wants the functionality, they only need a custom
template engine. They would need to do this anyway if they want to
load templates from any doc other than the default. That's what the
template engine extensibility is there for :)

Steve

On 23 Mar 2012, at 19:51, Michael Best
reply@reply.github.com
wrote:

@rniemeyer - A custom template source would not be able to handle it since it would run into the same problems we're having here, that the template engine needs to know what document to load the template from. This can only be solved in the core library. I'll look over what Steve has done and see if it can be made more compact.


Reply to this email directly or view it on GitHub:
#405 (comment)

@mbest

This comment has been minimized.

Copy link
Member

mbest commented Mar 23, 2012

I made the changes much more compact by using an extra parameter for the document (instead of a property of options) and only caching the re-written status for the main document. With this version, the minified code is 110 bytes larger (compared to 298 larger before).

@mbest

This comment has been minimized.

Copy link
Member

mbest commented Mar 23, 2012

We could keep the change that passes the doc info into the template engine, but drop the changes to the template engine/source itself.

I think you will like what I've done. But for comparison, if I take out all the changes from templateEngine, we drop 57 bytes--about half of the added code.

@mbest mbest referenced this pull request Apr 4, 2012

Closed

2.1 release discussion #338

@SteveSanderson

This comment has been minimized.

Copy link
Contributor

SteveSanderson commented Apr 10, 2012

Thanks Michael.

There was a further typo in commit 5051ea6 so I fixed that before the merge.

After all this discussion about caching the "isRewritten" status, I'm pretty convinced that the caching itself isn't worth keeping - it only applies in the case of rewritable templates (not 2.0-style native templates) and even then only benefits certain scenarios in old versions of IE. For KO 2.2, I'd prefer to drop this logic entirely to clean up that section of the code. Have submitted #427.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment