remove function export from helper blueprint

[kellyselden]

I find myself removing this export from every helper I create. Calling this function from component JS is cumbersome as you have to conform with its strict helper argument style. If I ever need to call a helper in JS, I move the code to a util and have the helper also use it.

[locks]

I disagree with this change, but in the case it is accepted, why have the function at all then?

[locks]

Aren't you also missing the necessary tweaks to the generated tests?
With this change, you can no longer unit-test a helper.

[kellyselden]

Correct. If accepted I will remove the unit test for helpers. Why do you disagree @locks? Calling the function behind the template helper seems like an anti-pattern to me.

[kellyselden]

@locks ping

[rwjblue]

For background, I believe that the main reason we do this is because at the time the integration testing system didn't support actually rendering a custom template, so it was nearly impossible to test properly from template land. Unfortunately, we have quite a large divide between the "pure helpers" and "class based helpers" with the current setup. I suspect that that the right path forward may be something of a compromise here.

For example, rwjblue/rfc232-demo-app#1 (thanks @spencer516!) that shows a really elegant setup for doing both (leveraging the new ember-qunit API of course):

module('helper:sad-color', function() {

  module('rendering tests', function(hooks) {
    setupRenderingTest(hooks);

    test('it renders', async function(assert) {
      await render(hbs`{{sad-color}}`);

      assert.equal(this.$().text().trim(), '#fff');
    });
  });

  module('unit tests', function(hooks) {
    setupTest(hooks);

    test('it returns black', function(assert) {
      const subject = this.owner.lookup('helper:sad-color');

      assert.equal(subject.compute(), '#fff');    });
  });
});

Neither case requires the current export function helper() {} shenanigans that is being removed here and we retain the ability to "unit test" the helpers nicely (and those tests work the same for class based and pure function helpers).

@locks / @kellyselden / @Turbo87 - Thoughts?

[kellyselden]

Seems good to me. In the beginning you mention a compromise, then at the end you seem to say remove the function export is fine. Does this PR need to change in your opinion @rwjblue ?

[rwjblue]

If this is the path forward then we should probably not even have the function separated at all. No?

[kellyselden]

I'm indifferent to it being separate. It might have a long line if we keep the function name for stack tracing purposes.

[Turbo87]

if "unit" testing via subject.compute() works for both function- and class-based helpers I'm okay with this change. On the other hand we might want to wait with this until we change the helper test blueprints to use the new APIs?

[rwjblue]

Confirm. I’d prefer to land only bugfixes for this stuff at the moment. Saving larger refactors in general until we land new blueprints.

[kellyselden]

Can y'all keep this on your radar with your test reworkings?

[Turbo87]

@kellyselden see also ember-codemods/ember-qunit-codemod#17

[Turbo87]

@kellyselden wanna revisit this?

[ef4]

I still think this is a good idea. Helpers should be tested via rendering tests, not unit tests, which is already the current default. So the original rationale for the named export doesn't apply.

I would also add that the re-exports of these named exports in the app tree tend to bit rot, because people aren't using them and don't notice when they break them. This generates warning spew in Embroider, which does notice invalid reexports.

For example, this:

https://github.com/jmurphyau/ember-truth-helpers/blob/master/app/helpers/not-eq.js

refers to a name that no longer exists in:

https://github.com/jmurphyau/ember-truth-helpers/blob/master/addon/helpers/not-equal.js

So every app using ember-truth-helpers ships a bit of unused-and-broken code.

[kellyselden]

Rebased. I still feel like this should be merged.

Side-note, there appears to be a node-tests regression on master. Another PR has the same 31 failed tests.

[pzuraq]

@kellyselden think you can rebase this again? I believe the node tests problem has been resolved

[kellyselden]

Rebased.

[rwjblue]

If we are going to remove the export, we should also inline the function.

export default helper(function(params /*, hash */) {
  return params;
})

[rwjblue]

@kellyselden - Only had that last tweak/suggestion, then this is good to merge...

[kellyselden]

Should it still be a named function for stack trace benefits?

[rwjblue]

@kellyselden ya, that makes sense to me

So basically:

export default helper(function fooBarBaz(params /*, hash */) {
  return params;
})

[Turbo87]

do we require named classes too from now on?

[rwjblue]

do we require named classes too from now on?

Seems unrelated. I don't think this is about what we require, it's about what we think is a good idea. Having the name of the class or function in the stack trace is very very useful...

[kellyselden]

Should be good now.