Broccoli coffee upgrade #124

jakesjews · 2016-10-31T01:09:45Z

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

Fixes #126

mriska · 2016-11-22T08:03:30Z

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

perlun · 2016-11-24T08:51:52Z

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

mriska · 2016-11-25T08:48:14Z

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 · 2016-11-25T15:43:34Z

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

mriska · 2016-11-25T19:16:51Z

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

mriska · 2016-11-25T21:43:34Z

@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 · 2016-11-26T19:11:27Z

@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 · 2016-11-26T19:25:13Z

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

jakesjews · 2016-11-26T19:26:53Z

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

jakesjews · 2016-11-26T19:30:54Z

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

jakesjews · 2016-11-26T19:31:32Z

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

mriska · 2016-11-26T19:33:39Z

@jakesjews I was thinking mainly about the templates in this commit: 452f83d

jakesjews · 2016-11-26T19:34:52Z

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

mriska · 2016-11-26T19:35:48Z

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

mriska · 2016-11-27T21:55:09Z

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:

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 · 2016-11-28T06:56:11Z

I have been reading up some more on tagged template literals, and from what I understand, this PR jashkenas/coffeescript#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 · 2016-11-29T19:48:09Z

@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 · 2016-11-30T11:48:10Z

@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 · 2016-12-09T00:56:36Z

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

mriska · 2016-12-09T07:18:47Z

@jakesjews awesome, thank you for carrying this forward.

jakesjews · 2017-01-09T17:05:01Z

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

mriska · 2017-01-09T19:53:19Z

Awesome work @jakesjews 🥇

jakesjews · 2017-01-31T04:22:28Z

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

kimroen · 2017-02-01T22:46:22Z

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: #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: #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: d397293

jakesjews · 2017-02-01T23:22:12Z

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

…cript into broccoli-coffee-upgrade

jakesjews · 2017-02-02T01:06:51Z

@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 #132 when I get some free time.

kimroen · 2017-02-02T10:13:00Z

node-tests/blueprints/acceptance-test-test.js

-          .to.contain("`import Ember from 'ember'`")
-          .to.contain("`import { module, test } from 'qunit'`")
-          .to.contain("`import startApp from 'my-app/tests/helpers/start-app'`")
+          .to.contain("import Ember from 'ember")


The ending singlequote after ember is missing here.

Nice catch fixed now. Although now I'm concerned that the tests passed even with the typo

It's just checking that the file contains that string, and 'ember' does contain 'ember. I think it's fine.

ha makes perfect sense still the morning for me 🤦‍♂️

kimroen · 2017-02-02T10:14:24Z

node-tests/blueprints/addon-import-test.js

@@ -16,7 +16,7 @@ describe('Acceptance: ember generate and destroy addon-import', function() {
    return emberNew()
      .then(() => emberGenerateDestroy(args, (file) => {
        expect(file('app/addon-imports/foo.coffee'))
-          .to.contain(`export { default } from 'my-app/addon-imports/foo'`);


Nice catch 👍

jakesjews · 2017-02-06T00:08:25Z

@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 · 2017-02-06T13:52:07Z

node-tests/blueprints/component-addon-test.js

@@ -15,8 +16,12 @@ describe('Acceptance: ember generate and destroy component-addon', function() {

    return emberNew({target: 'addon'})
      .then(() => emberGenerateDestroy(args, (file) => {
+        var componentFile = file('app/components/foo-bar.coffee');
+
        expect(file('app/components/foo-bar.coffee'))


Missed a spot - this should refer to componentFile.

kimroen · 2017-02-06T13:56:49Z

node-tests/helpers/expect-coffee.js

+    coffeescript.compile(file.content);
+  }
+
+  expect(compileFunc).to.not.throw(Error);


Have you verified that this actually throws Error if it fails?

yup! Atleast running expectCoffee('asjdnasdjk'); does

Running the literal expectCoffee('asjdnasdjk'); might throw an error for the wrong reasons though - on line 6 it would call 'asjdnasdjk'.content and get undefined, which is then passed into coffeescript.compile(). Would you mind testing by passing in a random string to coffeescript.compile() directly instead?

no problem. Just tried it out with a random string but apparently thats valid coffeescript so it doesn't throw. However calling coffeescript.compile('var x = 1;'); does throw an error. Do you want me to add a unit test in for the helper?

Do you want me to add a unit test in for the helper?

That's a great idea. Yes please.

@kimroen just pushed out some tests

kimroen · 2017-02-06T14:15:36Z

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 · 2017-02-07T19:57:25Z

@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 · 2017-02-07T19:58:31Z

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

kimroen · 2017-02-07T21:28:58Z

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

jakesjews added 2 commits October 30, 2016 20:53

update broccoli-coffee to 0.7.0

ccfe851

backticks are not needed anymore

d15c447

kimroen mentioned this pull request Nov 24, 2016

Update broccoli-coffee dependency to 0.7.0 #126

Closed

jakesjews added 2 commits November 26, 2016 13:50

remove semicolons from component blueprints

9bba9d4

restore backticks when default is used as an alias

452f83d

kimroen mentioned this pull request Dec 1, 2016

Update README now that import/export are in CoffeeScript #127

Closed

GeoffreyBooth mentioned this pull request Dec 5, 2016

Allow imported module members to be named default jashkenas/coffeescript#4388

Merged

kimroen mentioned this pull request Dec 10, 2016

Update broccoli-coffee minor version. #128

Closed

bump broccoli-coffee

7ae54de

jakesjews added 2 commits February 1, 2017 19:55

Merge branch 'master' of https://github.com/kimroen/ember-cli-coffees…

ca45f6b

…cript into broccoli-coffee-upgrade

remove backticks from blueprint tests

0a320f8

kimroen requested changes Feb 2, 2017

View reviewed changes

jakesjews added 3 commits February 2, 2017 09:16

fix typo

6f4e394

verify coffeescript in blueprint tests

ccbd0f0

add coffee-script to dev dependencies

7e7798f

kimroen approved these changes Feb 6, 2017

View reviewed changes

kimroen requested changes Feb 6, 2017

View reviewed changes

jakesjews added 2 commits February 6, 2017 09:26

fix variable reference

a281497

add tests for expect-coffee helper

5bdf4c2

kimroen approved these changes Feb 6, 2017

View reviewed changes

kimroen merged commit 61f3897 into kimroen:master Feb 6, 2017

kimroen mentioned this pull request Feb 6, 2017

Test that generated blueprints are valid CoffeeScript #132

Closed

kimroen added a commit that referenced this pull request Feb 6, 2017

Update changelog for #123, #124, and #129

e7a13cd

Broccoli coffee upgrade #124

Broccoli coffee upgrade #124

Conversation

jakesjews commented Oct 31, 2016 • edited by kimroen Loading

mriska commented Nov 22, 2016

perlun commented Nov 24, 2016

mriska commented Nov 25, 2016 • edited Loading

jakesjews commented Nov 25, 2016

mriska commented Nov 25, 2016

mriska commented Nov 25, 2016

jakesjews commented Nov 26, 2016 • edited Loading

mriska commented Nov 26, 2016

jakesjews commented Nov 26, 2016

jakesjews commented Nov 26, 2016

jakesjews commented Nov 26, 2016

mriska commented Nov 26, 2016

jakesjews commented Nov 26, 2016

mriska commented Nov 26, 2016

mriska commented Nov 27, 2016

mriska commented Nov 28, 2016

jakesjews commented Nov 29, 2016

mriska commented Nov 30, 2016

jakesjews commented Dec 9, 2016

mriska commented Dec 9, 2016

jakesjews commented Jan 9, 2017

mriska commented Jan 9, 2017

jakesjews commented Jan 31, 2017

kimroen commented Feb 1, 2017

jakesjews commented Feb 1, 2017

jakesjews commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesjews commented Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimroen Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimroen commented Feb 6, 2017

mriska commented Feb 7, 2017

mriska commented Feb 7, 2017

kimroen commented Feb 7, 2017

jakesjews commented Oct 31, 2016 •

edited by kimroen

Loading

mriska commented Nov 25, 2016 •

edited

Loading

jakesjews commented Nov 26, 2016 •

edited

Loading

jakesjews commented Feb 6, 2017 •

edited

Loading

kimroen Feb 6, 2017 •

edited

Loading