kimroen / ember-cli-coffeescript

Adds precompilation of CoffeeScript files and all the basic generation types to the ember generate command.
MIT License
72 stars 49 forks source link

Upgrade CoffeeScriptLinter to broccoli-persistent-filter #95

Closed johnnyshields closed 8 years ago

johnnyshields commented 8 years ago

This is the latest API from Ember team (see https://github.com/ember-cli/ember-cli/issues/5030) and the change below cuts my build time in half. I've tested this on a large CoffeeScript ember project.

johnnyshields commented 8 years ago

@kimroen please look at this, makes a significant improvement in build time.

jonnii commented 8 years ago

:+1:

stefanpenner commented 8 years ago

just made quick review, this LGTM

stefanpenner commented 8 years ago

ah, this should likely enable persist: true so fast warm boots are possible

kimroen commented 8 years ago

All right, tried all this out and it's looking really good :) Would you mind making the change above? After that I'll try out again to confirm, and then merge and release. Hopefully I can get that done tomorrow :)

kimroen commented 8 years ago

Thanks, and thanks for your help, stef :smile:

johnnyshields commented 8 years ago

@kimroen done.

johnnyshields commented 8 years ago

@kimroen the change as suggested broke our build, so I've rolled it back. Can we merge this PR as-is and then implement this persist option separately, as I don't know a lot about this .persist option and I'm really not the right person to implement this.

johnnyshields commented 8 years ago

FYI here's the log which occurred on ref ab59e5434bc534e5dad428c7a4fa7f07cb1a041e

BuildingCannot find module '/workspace/node_modules/ember-cli-coffeescript/lib//package.json'
Error: Cannot find module '/workspace/node_modules/ember-cli-coffeescript/lib//package.json'
  at Function.Module._resolveFilename (module.js:336:15)
  at Function.Module._load (module.js:278:25)
  at Module.require (module.js:365:17)
  at require (module.js:384:17)
  at pkg (/workspace/node_modules/ember-cli-coffeescript/node_modules/broccoli-persistent-filter/node_modules/hash-for-dep/lib/pkg.js:19:20)
  at statPathsFor (/workspace/node_modules/ember-cli-coffeescript/node_modules/broccoli-persistent-filter/node_modules/hash-for-
stefanpenner commented 8 years ago

looks like hash-for-dep is unable to resolve coffeescripts package.json

johnnyshields commented 8 years ago

@stefanpenner can we make that a separate issue please and merge this as-is.

stefanpenner commented 8 years ago

@stefanpenner can we make that a separate issue please and merge this as-is.

im indifferent (and also not a maintainer of the project, so can't really merge either way :P) but without persistence, it is still a really good win. So my vote is +1

opened an issue, will investigate the failure you describe soon (maybe tomorrow) ->https://github.com/stefanpenner/hash-for-dep/issues/21

johnnyshields commented 8 years ago

@kimroen please merge?

thomassnielsen commented 8 years ago

@johnnyshields could you please stop nagging? This project (as most open source projects) is maintained by volunteers, and as such may have limited time and have no obligation to look at issues and pull requests this often. It will be looked at in time. If you need this change, you can run your own fork of the project until it is considered and possibly merged.

kimroen commented 8 years ago

Took a brief look at the error, seems likely that this part is causing the problem:

CoffeeScriptLinter.prototype.baseDir = function() {
  return __dirname;
};

This file is not in the root of the project, so the package.json file wouldn't be there. Maybe this:

CoffeeScriptLinter.prototype.baseDir = function() {
  return path.resolve(__dirname, '..');
};

I'll try to merge and try that later today.

stefanpenner commented 8 years ago

@kimroen good catch!

johnnyshields commented 8 years ago

Tried @kimroen's fix, but looks like it's getting a different error:

version: 1.13.8
Cannot find module 'timers-ext/' from '\workspace\node_modules\ember-cli\node_modules\inquirer\node_modules\cli-color\node_modules\memoizee\'

Error: Cannot find module 'timers-ext/' from '\workspace\node_modules\ember-cli\node_modules\inquirer\node_modules\cli-color\node_modules\memoizee\'
  at Function.module.exports (\workspace\node_modules\ember-cli-coffeescript\node_modules\broccoli-persistent-filter\node_modules\hash-for-dep\node_modules\resolve\lib\sync.js:33:11)
  at resolvePkg (\workspace\node_modules\ember-cli-coffeescript\node_modules\broccoli-persistent-filter\node_modules\hash-for-dep\lib\resolve-pkg.js:19:18)
  at pkg (\workspace\node_modules\ember-cli-coffeescript\node_modules\broccoli-persistent-filter\node_modules\hash-for-dep\lib\pkg.js:17:20)
  at again (\workspace\node_modules\ember-cli-coffeescript\node_modules\broccoli-persistent-filter\node_modules\hash-for-dep\lib\deps-for.js:18:22)
  at \workspace\node_modules\ember-cli-coffeescript\node_modules\broccoli-persistent-filter\node_modules\hash-for-dep\lib\deps-for.js:27:7
  at Array.forEach (native)
kimroen commented 8 years ago

Merging this now, then I'll take a look at the problem with persist, and then release with or without it depending.

kimroen commented 8 years ago

Published without the persist option as v.1.13.2. Thanks again for your help, @johnnyshields!