Skip to content

Upgrade CoffeeScriptLinter to broccoli-persistent-filter#95

Merged
kimroen merged 4 commits intokimroen:masterfrom
johnnyshields:broccoli-persistent-filter
Nov 22, 2015
Merged

Upgrade CoffeeScriptLinter to broccoli-persistent-filter#95
kimroen merged 4 commits intokimroen:masterfrom
johnnyshields:broccoli-persistent-filter

Conversation

@johnnyshields
Copy link
Contributor

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

@johnnyshields johnnyshields force-pushed the broccoli-persistent-filter branch from a889d5c to 7ea8445 Compare November 8, 2015 01:20
@johnnyshields
Copy link
Contributor Author

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

@jonnii
Copy link

jonnii commented Nov 9, 2015

👍

@stefanpenner
Copy link

just made quick review, this LGTM

@stefanpenner
Copy link

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

Copy link
Owner

Choose a reason for hiding this comment

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

Could you set the options.persist = true, and then move the Filter.call line to after this?

@kimroen
Copy link
Owner

kimroen commented Nov 9, 2015

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
Copy link
Owner

kimroen commented Nov 9, 2015

Thanks, and thanks for your help, stef 😄

@johnnyshields
Copy link
Contributor Author

@kimroen done.

@johnnyshields
Copy link
Contributor Author

@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
Copy link
Contributor Author

FYI here's the log which occurred on ref ab59e54

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
Copy link

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

@johnnyshields
Copy link
Contributor Author

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

@stefanpenner
Copy link

@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) ->ember-cli/hash-for-dep#21

@johnnyshields
Copy link
Contributor Author

@kimroen please merge?

@thomassnielsen
Copy link

@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
Copy link
Owner

kimroen commented Nov 10, 2015

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
Copy link

@kimroen good catch!

@johnnyshields
Copy link
Contributor Author

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
Copy link
Owner

kimroen commented Nov 22, 2015

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

kimroen added a commit that referenced this pull request Nov 22, 2015
Upgrade CoffeeScriptLinter to broccoli-persistent-filter
@kimroen kimroen merged commit e5710ed into kimroen:master Nov 22, 2015
kimroen added a commit that referenced this pull request Nov 22, 2015
@kimroen
Copy link
Owner

kimroen commented Nov 22, 2015

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

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.

5 participants