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

[Glimmer2] tests for html-safe and render #13845

Merged
merged 1 commit into from Jul 20, 2016

Conversation

chadhietala
Copy link
Contributor

/cc @krisselden @rwjblue

Pretty sure we want to just use the SafeString from HandleBars, however it is unclear how we should import it sense it was a proxy import in HTMLBars.

@rwjblue
Copy link
Member

rwjblue commented Jul 19, 2016

@chadhietala - This is good to me. Can you make https://github.com/emberjs/ember.js/blob/master/packages/ember-htmlbars/lib/utils/string.js a symlink to the ember-glimmer one, and expose it to Ember.Handlebars and Ember.HTMLBars in ember-templates?

@@ -0,0 +1 @@
../../../ember-htmlbars/lib/utils/string.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should go the other way since in the long term we are going to rm -rf packages/ember-htmlbars.

@rwjblue
Copy link
Member

rwjblue commented Jul 19, 2016

Overall this look good (one comment/tweak), can you confirm that escapeExpression is exposed by glimmer's build?

@chadhietala
Copy link
Contributor Author

What I ended up doing is moving the string module into ember-glimmer and also inlining the SafeString class and escapExpression functions as they came from HTMLBars. SafeString in HTMLBars is just a proxy to Handlebars.

Ember.HTMLBars = { SafeString };
}

if (Ember.HandlerBars) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Ember.Handlebars (a few places below are typo'ed too).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also move these Ember.Handlebars exports to ember-templates/compat?

@chadhietala chadhietala changed the title [Glimmer2] tests for html-safe, actions, and render [Glimmer2] tests for html-safe and render Jul 20, 2016
@chadhietala
Copy link
Contributor Author

Had to do some reorg and deletion of some similar work. Also moved the action test to #13850.

EmberHTMLBars.template = EmberHandlebars.template = template;
EmberHTMLBarsUtils.escapeExpression = EmberHandleBarsUtils.escapeExpression = escapeExpression;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not believe that we ever exposed this previously on Ember.HTMLBars, can you confirm and if we haven't fixup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Maintain our own safestring and escapeexpression for now
@rwjblue
Copy link
Member

rwjblue commented Jul 20, 2016

LGTM

@chadhietala chadhietala merged commit 52ba7d0 into emberjs:master Jul 20, 2016
@chadhietala chadhietala deleted the rando-tests branch July 20, 2016 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants