-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Bootstrap DOM templates into the container instead of Ember.TEMPLATES #3487
Conversation
Definitely would like to hear from @wycats. We don't worry about breaking abuses of the system, we just don't want to break stuff that we said would work. |
// Check if template of same name already exists | ||
if (Ember.TEMPLATES[templateName] !== undefined) { | ||
throw new Error('Template named "' + templateName + '" already exists.'); | ||
if (container) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when don't we have a container.
Unfortunately, we really did document @lukemelia thoughts? |
@lukemelia, ping. |
@wycats regarding your suggestion of an API that would be used for precompiled templates... in the ember-app-kit approach, you precompile to modules, which are then resolved on demand. If we want a solution for the current DefaultResolver approach, it seems like we could just continue to support Ember.TEMPLATES, instead of creating something new. For both approaches though, I think it is a win to register with the container when bootstrapping templates from HTML. I can update the PR so that templates are also added to Ember.TEMPLATES. Does that seem like a win? |
@wycats, @lukemelia what needs to happen for this to progress? |
I think we should bootstrap into the container only, but update the resolver to check I believe this approach would be backwards compatible since folks that are manually adding templates to @lukemelia - Thoughts? |
@rjackson sounds good to me. If we have agreement, I'd be happy to expand this PR to include those changes. |
@rjackson that idea sounds good to me. |
@lukemelia - I think we are all on the same page here (I don't see any disagreement to my proposal). Need any other input to pick this back up again? |
Sounds good. I will update this PR accordingly. |
After digging into this, I have discovered a few issues with the planned approach.
Long story short, item 1 above is something we can work with for now, but item 2 is a blocker to implementing the change contemplated here. |
Stuff has changed a lot in the last year+. @emberjs/owners thoughts? |
Pretty sure we can close this; ember-cli does this automatically and we have test cases against Ember.TEMPLATES to make sure we don't break people still in globals mode. |
This is almost certainly the direction we want to go.
But I am unsure about backward compatibility and whether we should feature flag this.
For normal usage, this change should be transparent. Not sure if people may be abusing this method in some way that requires leaving in the no container branch as I have here.
Feedback?