Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaced coffescript task with grunt-coffee #522

Closed
wants to merge 6 commits into from

Conversation

blakeblackshear
Copy link
Contributor

This replaces the coffee task with grunt-coffee, it has several features that grunt-contrib-coffee doesn't. The biggest one is the ability to maintain a directory structure when compiling the app directory to the temp directory. This addresses issues #407, #387.

I have also opened a pull request to update the generators as well.

@blakeblackshear
Copy link
Contributor Author

I have signed the CLA

@sindresorhus
Copy link
Member

Thanks, but you're using grunt-coffee, instead of grunt-contrib-coffee. Can you change that?

Also make sure the test suite passes locally. I know Travis fails now because it depends on the generator PR, but it should pass locally with both changes.

Sidenote: In the future, please notice if the issue is assigned or not. I was actually in the middle of making this change.

Also link to the other PR by link. GitHub will make it pretty. This makes us not having to hunt it down.

@sindresorhus
Copy link
Member

Also, issue 385 looks like the wrong reference.

@blakeblackshear
Copy link
Contributor Author

I can look again, but i didn't see the ability to preserve the directory
structure with the contrib project.
On Sep 22, 2012 8:26 AM, "Sindre Sorhus" notifications@github.com wrote:

Thanks, but you're using grunt-coffee, instead of grunt-contrib-coffee.
Can you change that?

Also make sure the test suite passes locally. I know Travis fails now
because it depends on the generator PR, but it should pass locally with
both changes.

Sidenote: In the future, please notice if the issue is assigned or not.
I was actually in the middle of making this change.

Also link to the other PR by link. GitHub will make it pretty. This makes
us not having to hunt it down.


Reply to this email directly or view it on GitHubhttps://github.com//pull/522#issuecomment-8788749.

@@ -55,7 +55,8 @@
"prompt": "~0.1.12",
"colors": "~0.6.0",
"grunt-mocha": "~0.1.3",
"es6-collections": ">=0.2.0"
"es6-collections": ">=0.2.0",
"grunt-coffee": "0.0.6"
Copy link
Member

Choose a reason for hiding this comment

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

"grunt-contrib-coffee": "~0.2.0"

@blakeblackshear
Copy link
Contributor Author

The TravisCI failure looks like something didnt fetch from bower correctly. The tests pass locally, and this pull request isn't dependent on the other one. The other one is dependent on this one.

@blakeblackshear
Copy link
Contributor Author

Tests also pass locally when I update the generators with my pull request.

@blakeblackshear
Copy link
Contributor Author

Unless I am missing something, grunt-contrib-coffee does not give me the ability to preserve the directory structure when compiled to temp. This will break rjs when it tries to trace the dependencies.

@sindresorhus
Copy link
Member

Unless I am missing something, grunt-contrib-coffee does not give me the ability to preserve the directory structure when compiled to temp. This will break rjs when it tries to trace the dependencies.

You might be right about that. However grunt-contrib-coffee is the official one, and grunt-coffee is probably going to be deprecated.

Can you add a PR on the grunt-contrib-coffee repo to add that? And we'll just pull in your fork for the time being.

@blakeblackshear
Copy link
Contributor Author

Will do.

@sindresorhus
Copy link
Member

@blakeblackshear Can you update to use the suggested method in the PR? Latest grunt-contrib-coffee is available on NPM.

@pheuter
Copy link

pheuter commented Sep 24, 2012

@sindresorhus Tried using the suggested method via grunt-contrib-coffee, getting the following warning when building Yeoman project:

Running "coffee:dist" (coffee) task
<WARN> Unable to write "app/scripts/compiled" file (Error code: EISDIR). Use --force to continue. </WARN>

Aborted due to warnings.

In Gruntfile.js, I have:

coffee: {
      dist: {
        src: 'app/scripts/**/*.coffee',
        dest: 'app/scripts/compiled',

        options: {
          base_path: 'app/scripts',
          preserve_dirs: true
        }
      }
},

@blakeblackshear
Copy link
Contributor Author

Tests will fail until this generators pull request is in: https://github.com/yeoman/generators/pull/62
I have verified that they pass locally.

@sindresorhus
Copy link
Member

Landed. Keep up the good work @blakeblackshear :D

@sindresorhus
Copy link
Member

@blakeblackshear Looks like yeoman server isn't working after this PR:

Running "watch" task
Verifying properties watch.coffee.files, watch.coffee.tasks exist in config...ERROR
>> Unable to process task.
<WARN> Required config property "watch.coffee.files" missing. Use --force to continue. </WARN>

Could you take a look?

@addyosmani
Copy link
Member

Confirmed.

Running "watch" task
Verifying properties watch.coffee.files, watch.coffee.tasks exist in config...ERROR
>> Unable to process task.
<WARN> Required config property "watch.coffee.files" missing. Use --force to continue. </WARN>

Aborted due to warnings.
addyo at dhcp-172-16-23-68 in ~/projects/test/556

@blakeblackshear
Copy link
Contributor Author

Looking into it. The contrib coffee task also fails if there are no coffee files, not sure what is the best way to handle that.

@sindresorhus
Copy link
Member

The contrib coffee task also fails if there are no coffee files, not sure what is the best way to handle that.

Fixed that some hours ago. Pull and npm install.

@blakeblackshear
Copy link
Contributor Author

Thanks. The problem is the config for the watch coffee task. It is trying to lookup from <config:coffee.dist.src>, but the new coffee config looks like this:

coffee: {
      compile: {
        files: {
          'temp/scripts/*.js': 'app/scripts/**/*.coffee'
        },
        options: {
          basePath: 'app/scripts'
        }
      }
    }

It can be fixed by just putting 'app/scripts/**/*.coffee' instead of <config:coffee.dist.src>, but I am sure it would be better to look it up. I am not sure how to do a lookup on the new coffee config.

@sindresorhus
Copy link
Member

It would be better to look it up, but we'll go for the temporary fix for now, just to get an release out.

Can you open an issue about this to get it fixed for real, so we don't forget?

@blakeblackshear
Copy link
Contributor Author

Should I also issue a pull request with the temporary fix on the generators repo?

@addyosmani
Copy link
Member

No worries. I've tested the fix and am making the updates to the generators now. Need to test this works with all of them.

@blakeblackshear
Copy link
Contributor Author

Ok. Thanks. Sorry for breaking the build.

@sleeper
Copy link
Contributor

sleeper commented Sep 26, 2012

I confirm it's working.

Just a question on top of my head: Shouldn't we add something related to this change in the FAQ (i.e. to indicate how the Gruntfile.js needs to be changed, for people that had Gruntfile.js instantiated with previous versions of yeoman) ?

@chadhietala
Copy link

+1 To fixing this. +1 for putting this in the FAQs. You may also want to add that you need to run npm install grunt-contrib-coffee in /usr/local/lib/node_modules/yeoman/node_modules/.

@sindresorhus
Copy link
Member

Agree on fixing and faq.

The last point is incorrect. If you upgrade though npm it's done automatically. If you use master, you can just do 'mom install' in the /cli folder

@sleeper
Copy link
Contributor

sleeper commented Sep 26, 2012

@sindresorhus I've added an entry in the Additional FAQ ("Q: I just upgraded to 0.9.1 and my coffee files are not compiled anymore" near end of the FAQ)

@sindresorhus
Copy link
Member

@sleeper Much appreciated :)

@chadhietala
Copy link

@sindresorhus When yeoman autoupdated to 0.9.2 it complained that I didn't have the package.

Shouldn't the FAQs reference upgrading to 0.9.2 not 0.9.1?

@sleeper
Copy link
Contributor

sleeper commented Sep 27, 2012

Actually looking at the changelog, this change has been part of 0.9.1.

szinya pushed a commit to menthainternet/yeoman that referenced this pull request Sep 17, 2014
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.

None yet

6 participants