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

Expose helpers under app/helpers/ directory to allow auto-registering them in 1.13 #12

Merged
merged 3 commits into from
Aug 10, 2015

Conversation

irnc
Copy link

@irnc irnc commented Jul 2, 2015

Starting from ember 1.13.0 those helpers would be auto-registered and usable inside new component integration tests

@jmurphyau
Copy link
Owner

Can you please provide some context/more info around what you're talking about? Where did you get this info? PR/blog post/etc

@irnc
Copy link
Author

irnc commented Jul 6, 2015

Please see emberjs/ember-test-helpers#38 (comment) and one below it elaborating on "reason it's easier to stick with the naming conventions."

It also says that Unfortunately helpers without a dash are not auto discovered. which was true as of May 3rd, but no more from June 12th. See blog post for 1.13.0 release, especially https://github.com/mixonic/rfcs/blob/helper-listing/active/0000-helper-listing.md

@irnc irnc changed the title Expose correct helpers under app/helpers/ directory Expose helpers under app/helpers/ directory to allow auto-registering them in 1.13 Jul 24, 2015
@irnc
Copy link
Author

irnc commented Jul 24, 2015

Please see emberjs/ember.js#11393 for PR which implements needed functionality starting from Ember 1.13:

With this change, all helpers that are known to the registry or resolver are whitelisted automatically and are not subject to the former dash requirement.

@irnc irnc force-pushed the expose-correct-app-helpers branch from 5377624 to a07810e Compare July 24, 2015 14:38
@irnc
Copy link
Author

irnc commented Jul 24, 2015

@jmurphyau I just remade proposed change based on your master branch. Could you please have another look at this pull request and emberjs/ember.js#11393.

This change does not remove existing initializer, so truth helpers should continue to work with older Ember releases, while also available in new component integration tests without prior manual registration.

@rwjblue
Copy link
Collaborator

rwjblue commented Jul 28, 2015

We should also make the initializer a noop when running under 1.13. As of emberjs/ember.js#11900 (which will be in Ember 1.13.6) all forms of helper registration other than auto-resolution is deprecated. Ember 2.0 (current beta) and 2.1 (current canary) will remove support for many other helper mechanisms (only Ember.HTMLBars.makeBoundHelper will remain but it is deprecated).

@rwjblue
Copy link
Collaborator

rwjblue commented Jul 28, 2015

Also, the test suite failure here will be fixed by an updated ember-qunit (0.4.5 or newer).

@jmurphyau
Copy link
Owner

@irnc can you please rebase? Also, Ember.Helper.helper is new so you might want to consider some type of check before using it (if (Ember.Helper) { Ember.Helper.helper() } - not sure if that will work/cover it tho? Might do)

@rwjblue
Copy link
Collaborator

rwjblue commented Jul 31, 2015

Checking for Ember.Helper should be good.

pzubkou added 3 commits August 4, 2015 11:55
Starting from ember 1.13.0 those helpers would be auto-registered and usable inside new component integration tests

See emberjs/ember.js#11393 for details
@irnc irnc force-pushed the expose-correct-app-helpers branch from a07810e to 06e0041 Compare August 4, 2015 11:51
import Ember from 'ember';
import { andHelper } from 'ember-truth-helpers/helpers/and';

export default Ember.Helper.helper(andHelper);
Copy link
Author

Choose a reason for hiding this comment

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

Note that we don't check for Ember.Helper here because this files should be auto-discovered only in Ember 1.13, which has Ember.Helper API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood, but:

  • it doesn't hurt to guard
  • helpers like the is-array helper would be discovered automatically (because of the dash) in Ember 1.12
  • it makes us feel better 😺

Copy link
Author

Choose a reason for hiding this comment

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

@rwjblue Ok, I will work on this check. Do you have any clue how we can test this guard? Right now tests does not catch this incompatibility with ember < 1.13.

Copy link
Author

Choose a reason for hiding this comment

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

helpers like the is-array helper would be discovered automatically (because of the dash) in Ember 1.12

I assume that helper registered in initializer will prevent auto-discovery process from happening for is-array, is this correct assumption?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume that helper registered in initializer will prevent auto-discovery process from happening for is-array, is this correct assumption?

Yep.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have any clue how we can test this guard?

Its mostly to make us feel better (I know we are silly), so I'm fine with the fact that the tests all pass (which the do without the guard).

Copy link
Author

Choose a reason for hiding this comment

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

That means that prior to 1.13 application will receive helpers from initialize step, while for 1.13 all of them will be auto-discovered.

Taking this separation of concerns into account I propose we use Ember.Helper in app/helpers/* without prior check for it.

If it is not reasonable, I will use a few days to see how I can write tests for < 1.13

Thanks for help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

06e0041 definitely prevents the initializer from running in an application, however it does not prevent the tests in this repo from running registerHelper manually (see here for example). This means that the tests here are still using registerHelper and are unlikely to pass or work on Ember 2.0.0 (current beta cycle) once Ember.HTMLBars._registerHelper is removed (which will be happening within the next day or two).

If you move that guard into registerHelper instead of in the initializer that would more accurately reflect reality in the tests, but it will likely fail the tests because they are not setup with a resolver.

We need to restructure the tests to use moduleForComponent (from ember-qunit) like this example for the resolver stuff to be fixed. We should likely refactor the tests in a separate PR, then rebase this one on top of that once landed. I can tackle updating the tests later today/tomorrow unless you have a chance to do it earlier...

Copy link
Author

Choose a reason for hiding this comment

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

I will not promise that I will have time for restructuring tests for a few days ahead, so please take your time to do this.

@jmurphyau What is the plan for supported Ember versions? Will this project mimic what others do following Ember major.minor version? E.g 1.0.0 is compatible from Ember < 1.13, then release 1.13, and then 2.0 to remove deprecated API usage.

Copy link
Owner

Choose a reason for hiding this comment

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

@irnc I actually had no plans in regards to versioning like you've mentioned

@jmurphyau jmurphyau mentioned this pull request Aug 9, 2015
jmurphyau added a commit that referenced this pull request Aug 10, 2015
Expose helpers under app/helpers/ directory to allow auto-registering them in 1.13
@jmurphyau jmurphyau merged commit 77d67a3 into jmurphyau:master Aug 10, 2015
@jmurphyau
Copy link
Owner

@irnc thanks for this PR. I've merged it in and added that guard mentioned. Your changed are available in 1.1.0 (:

@irnc
Copy link
Author

irnc commented Aug 11, 2015

Thanks 👍

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.

3 participants