Broccoli coffee upgrade

[jakesjews]

This updates coffeescript to 1.11.1 and removes the need to use backticks for modules.

Fixes #126

[mriska]

It would be really awesome to get this merged. I'll try this out on my computer to confirm that it works.

[perlun]

@mriska - did you have a chance to test it?

[mriska]

I was testing this out today and I did run into an issue when using the new version in an add-on. In a normal ember project it looks like it is working, but in two addons I tested with, I got the following error:

➜  coffee-test git:(master) ✗ ember test
WARNING: Node v7.1.0 has currently not been tested against Ember CLI and may result in unexpected behaviour.
cleaning up...
Build failed.
File: dummy/components/bar-bar.coffee (1)
The Broccoli Plugin: [CoffeeScriptFilter] failed with:
[stdin]:1:1: error: reserved word 'export'
export { default } from 'coffee-test/components/bar-bar'
^^^^^^

I had a component in the addon that I tried to use in the dummy app in the addon.

It also looks like at least the component generator is generating code which is a mix of coffeescript and javascript:

import Ember from 'ember'
import layout from '../templates/components/bar-bar';

Note: the semicolon at the end.

The contents for <%= importTemplate %> seems to stem from ember-cli-legacy-blueprints/blueprints/component/index.js

[jakesjews]

@mriska can you send me a link to the test repo? I'll try fixing the issue today.

[mriska]

Hi @jakesjews, I'll try to get a test repo published for you later tonight.

[mriska]

@jakesjews I have pushed a repo with my test now: https://github.com/mriska/coffee-test Running ember test gives:

➜  coffee-test git:(master) ember test
WARNING: Node v7.1.0 has currently not been tested against Ember CLI and may result in unexpected behaviour.
cleaning up...
Build failed.
File: dummy/components/coffee-component.coffee (1)
The Broccoli Plugin: [CoffeeScriptFilter] failed with:
[stdin]:1:10: error: unexpected default
export { default } from 'coffee-test/components/coffee-component'
         ^^^^^^^

The broccoli plugin was instantiated at:
  at CoffeeScriptFilter.Plugin (/Users/mriska/work/coffee-test/node_modules/broccoli-plugin/index.js:7:31)
  at CoffeeScriptFilter.Filter [as constructor] (/Users/mriska/work/coffee-test/node_modules/broccoli-persistent-filter/index.js:60:10)
  at new CoffeeScriptFilter (/Users/mriska/work/coffee-test/node_modules/broccoli-coffee/index.js:16:10)
  at CoffeeScriptFilter (/Users/mriska/work/coffee-test/node_modules/broccoli-coffee/index.js:15:53)
  at CoffeePreprocessor.toTree (/Users/mriska/work/coffee-test/node_modules/ember-cli-coffeescript/lib/coffee-preprocessor.js:23:10)
  at /Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/ember-cli-preprocess-registry/preprocessors.js:180:26
  at Array.forEach (native)
  at processPlugins (/Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/ember-cli-preprocess-registry/preprocessors.js:178:11)
  at module.exports.preprocessJs (/Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/ember-cli-preprocess-registry/preprocessors.js:171:10)
  at EmberAddon.EmberApp.appAndDependencies (/Users/mriska/work/coffee-test/node_modules/ember-cli/lib/broccoli/ember-app.js:1087:25)
  at EmberAddon.EmberApp.javascript (/Users/mriska/work/coffee-test/node_modules/ember-cli/lib/broccoli/ember-app.js:1216:34)
  at EmberAddon.EmberApp.toArray (/Users/mriska/work/coffee-test/node_modules/ember-cli/lib/broccoli/ember-app.js:1640:10)
  at EmberAddon.EmberApp.toTree (/Users/mriska/work/coffee-test/node_modules/ember-cli/lib/broccoli/ember-app.js:1662:30)
  at module.exports (/Users/mriska/work/coffee-test/ember-cli-build.js:17:14)
  at Class.setupBroccoliBuilder (/Users/mriska/work/coffee-test/node_modules/ember-cli/lib/models/builder.js:70:19)
  at Class.init (/Users/mriska/work/coffee-test/node_modules/ember-cli/lib/models/builder.js:50:10)
  at Class.superWrapper [as init] (/Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/core-object/lib/assign-properties.js:32:18)
  at Class (/Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/core-object/core-object.js:32:33)
  at Class.run (/Users/mriska/work/coffee-test/node_modules/ember-cli/lib/tasks/build.js:15:19)
  at Class.<anonymous> (/Users/mriska/work/coffee-test/node_modules/ember-cli/lib/commands/test.js:164:27)
  at tryCatch (/Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/rsvp/dist/rsvp.js:538:12)
  at invokeCallback (/Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/rsvp/dist/rsvp.js:553:13)
  at /Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/rsvp/dist/rsvp.js:628:16
  at flush (/Users/mriska/work/coffee-test/node_modules/ember-cli/node_modules/rsvp/dist/rsvp.js:2373:5)
  at _combinedTickCallback (internal/process/next_tick.js:67:7)
  at process._tickCallback (internal/process/next_tick.js:98:9)

[jakesjews]

@mriska so it looks like theres a bug in coffeescript's module syntax where an error is being throw if default is being used like export { default }. I have a test failing in the coffee-script repo and hopefully will be able to make a PR in the near future to fix it. For now I restored the backticks to any blueprints which use export { default }.

I'm not sure if we should hold off until this is fixed upstream or for now just use backticks whenever we need to use export {default } which I don't think is too common in regular ember projects.

[mriska]

@jakesjews, If we reintroduce backticks around the exports, it should include a semi-colon also, right?

[jakesjews]

Whoops your right. I'll add that back in.

[jakesjews]

@mriska actually I'm having a little trouble finding where I forgot to restore the semi-colon where is it at?

[jakesjews]

Keep in mind export default is ok, its just export { default } that is the issue.

[mriska]

@jakesjews I was thinking mainly about the templates in this commit: https://github.com/kimroen/ember-cli-coffeescript/pull/124/commits/452f83dc7d442f4823177e1bf977cc93c4c5a426

[jakesjews]

@mriska I restored them back to what they were on master so I'm assuming they are ok.

[mriska]

Ok, I see that now. I just assumed that the export statement should have a semicolon, but I was mistaken.

[mriska]

I have retested with your changes and the exports now work correct. However, we run into a new problem:

➜  coffee-test git:(master) ember test
WARNING: Node v7.1.0 has currently not been tested against Ember CLI and may result in unexpected behaviour.
cleaning up...
Built project successfully. Stored in "/Users/mriska/work/coffee-test/tmp/core_object-tests_dist-GR5Iu3vK.tmp".
ok 1 PhantomJS 2.1 - JSHint | modules/coffee-test/components/js-component.js: should pass jshint
ok 2 PhantomJS 2.1 - JSHint | app.js: should pass jshint
ok 3 PhantomJS 2.1 - JSHint | helpers/destroy-app.js: should pass jshint
ok 4 PhantomJS 2.1 - JSHint | helpers/module-for-acceptance.js: should pass jshint
ok 5 PhantomJS 2.1 - JSHint | helpers/resolver.js: should pass jshint
ok 6 PhantomJS 2.1 - JSHint | helpers/start-app.js: should pass jshint
not ok 7 PhantomJS 2.1 - Integration | Component | coffee component2: it renders
    ---
        actual: >
            null
        message: >
            Died on test #1 http://localhost:7357/assets/tests.js:116:24
            exports@http://localhost:7357/assets/vendor.js:132:37
            requireModule@http://localhost:7357/assets/vendor.js:32:25
            require@http://localhost:7357/assets/test-support.js:7051:14
            loadModules@http://localhost:7357/assets/test-support.js:7043:21
            load@http://localhost:7357/assets/test-support.js:7073:33
            http://localhost:7357/assets/test-support.js:6956:22: Assertion Failed: Using a custom `.render` function is no longer supported.
        Log: |
    ...
ok 8 PhantomJS 2.1 - Integration | Component | js component: it renders
ok 9 PhantomJS 2.1 - JSHint | integration/components/js-component-test.js: should pass jshint
ok 10 PhantomJS 2.1 - JSHint | resolver.js: should pass jshint
ok 11 PhantomJS 2.1 - JSHint | router.js: should pass jshint
ok 12 PhantomJS 2.1 - JSHint | test-helper.js: should pass jshint

1..12
# tests 12
# pass  11
# skip  0
# fail  1
Not all tests passed.
Error: Not all tests passed.
  at App.getExitCode (/Users/mriska/work/coffee-test/node_modules/testem/lib/app.js:434:15)
  at App.exit (/Users/mriska/work/coffee-test/node_modules/testem/lib/app.js:189:23)
  at /Users/mriska/work/coffee-test/node_modules/testem/lib/app.js:103:14
  at tryCatcher (/Users/mriska/work/coffee-test/node_modules/testem/node_modules/bluebird/js/release/util.js:16:23)
  at Promise._settlePromiseFromHandler (/Users/mriska/work/coffee-test/node_modules/testem/node_modules/bluebird/js/release/promise.js:510:31)
  at Promise._settlePromise (/Users/mriska/work/coffee-test/node_modules/testem/node_modules/bluebird/js/release/promise.js:567:18)
  at Promise._settlePromise0 (/Users/mriska/work/coffee-test/node_modules/testem/node_modules/bluebird/js/release/promise.js:612:10)
  at Promise._settlePromises (/Users/mriska/work/coffee-test/node_modules/testem/node_modules/bluebird/js/release/promise.js:691:18)
  at Async._drainQueue (/Users/mriska/work/coffee-test/node_modules/testem/node_modules/bluebird/js/release/async.js:138:16)
  at Async._drainQueues (/Users/mriska/work/coffee-test/node_modules/testem/node_modules/bluebird/js/release/async.js:148:10)
  at Immediate.Async.drainQueues (/Users/mriska/work/coffee-test/node_modules/testem/node_modules/bluebird/js/release/async.js:17:14)
  at runCallback (timers.js:637:20)
  at tryOnImmediate (timers.js:610:5)
  at processImmediate [as _immediateCallback] (timers.js:582:5)

I believe this is due to how htmlbars-inline-precompile utilizes ES tagged template literals:

  this.render(hbs`{{js-component}}`);

which the coffeescript version of the test tries to use as:

  @render hbs """{{coffee-component2}}"""

These are obviously not the same thing and it looks like ES6 tagged template literals is simply not compatible with coffeescript at all at the moment:

• https://github.com/coffeescript6/discuss/issues/28
• https://github.com/jashkenas/coffeescript/issues/4301

I honestly don't know how to proceed on this issue. From my perspective, it looks like using coffeescript will be very difficult when tagged template literals is used and they are not supported in coffeescript.

[mriska]

I have been reading up some more on tagged template literals, and from what I understand, this PR https://github.com/jashkenas/coffeescript/pull/4352 that was merged into master a week ago supports it. I guess this means that 1.12 should support tagged template literals. We should probably wait for that to land?

@jakesjews: the other Coffeescript error that you mentioned you have a failing error for. Do you have a reference for that? Is it an uncontroversial fix that might be possible to get fixed quickly, and into 1.12?

[jakesjews]

@mriska It seems like htmlbars-inline-precompile should still work with coffeescript strings since I make pretty heavy use of it with the older version of cli-coffeescript.

The test I was referring to was a test I made locally in https://github.com/jashkenas/coffeescript/blob/master/test/modules.coffee.

test "export an alias named default", ->
  input = "export { default }"
  output = """
    export {
      default
    };"""
  eq toJS(input), output

I don't think the change in coffeescript should be controversial assuming that it is spec compliant (which I hope it is since ember is using it)

[mriska]

@jakesjews You are correct, the error I am seeing is probably due to something else. According to https://github.com/ember-cli/ember-cli-htmlbars-inline-precompile#coffeescript-support coffeescript should be supported using hbs as a regular method instead of a tagged template method :)

I will need to dig deeper to see what is causing this error.

[jakesjews]

It looks like the module fix is in coffeescript 1.12.1. After https://github.com/joliss/broccoli-coffee/pull/19 is merged we should be good to remove all backticks

[mriska]

@jakesjews awesome, thank you for carrying this forward.

[jakesjews]

Just pulled in the latest broccoli-coffee and it looks like the build is green now!

[mriska]

Awesome work @jakesjews 🥇

[jakesjews]

@kimroen is there anything I should do with this PR or are we good to merge now?

[kimroen]

Hi again @jakesjews, thanks for checking in (and awesome job on this!).

I haven't merged this because it's a pretty big diff and I wanted to make sure it all all still worked. I haven't gotten around to verifying this, because I don't have any projects that use this addon at the moment, so the time just isn't there.

That said I want to fix that, and a good way is to have automated tests for everything so I don't have to try everything manually. Thanks to the ember-cli-blueprint-test-helpers, there's now a convenient way to do that, so I have set up tests: https://github.com/kimroen/ember-cli-coffeescript/pull/129

I just have the very basic stuff so far (one test for each blueprint), but I think that's enough for a smoke test. What is missing though before I can merge this with confidence and avoid problems like the one @mriska found, is to also test that the CoffeeScript the blueprints generate are valid.

I made an issue describing what I'm looking for over here: https://github.com/kimroen/ember-cli-coffeescript/issues/132

I'll get to it myself (hopefully soon, but you know…), but if you'd like to have a stab at it then please go right ahead.

After that, the tests need to be updated on this branch to remove the backticks and pass, then I think it's good to go.

Thanks again!

---

The conflicting file in this PR has been renamed - that was done here: https://github.com/kimroen/ember-cli-coffeescript/commit/d3972934e5aa08712855f023e0c51559266aa1e9

[jakesjews]

@kimroen thanks for the response! I'll get those test merged and de-backticked.

[jakesjews]

@kimroen just merged with latest master. I removed the backticks from the node tests and they seem to be passing locally. I'll take a look at https://github.com/kimroen/ember-cli-coffeescript/issues/132 when I get some free time.

[jakesjews]

@kimroen I updated the node tests to verify that the coffee-script in the blueprints is valid. The tests are all passing on stable but it looks like beta scenario on CI is failing.

Running EMBER_TRY_SCENARIO=ember-beta ./scripts/run-tests.sh locally passes so I'm not sure if its a CI flap.

[kimroen]

The tests are all passing on stable but it looks like beta scenario on CI is failing. […] so I'm not sure if its a CI flap.

It is. It just fails on beta sometimes saying there were no tests to run, and then pass when I rerun. I have yet to figure out why.

[mriska]

@kimroen I was seeing those intermittent test failures in a private addon repo of ours as well. They seem to have disappeared since I changed from PhantomJS to Firefox as the test runner in Travis. It might be worth a try?

[mriska]

Great work @jakesjews, thanks for having the patience to see this through.

[kimroen]

@mriska Thanks for sharing your experience! I'll take a look :)