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

Remove grunt #6112

Closed
wants to merge 47 commits into from
Closed

Remove grunt #6112

wants to merge 47 commits into from

Conversation

noppanit
Copy link

As we don't need Grunt and Gulp, this PR attempts to replace Grunt with Gulp.

Related: #5829

let minifyCss = require('gulp-minify-css');
let stylus = require('gulp-stylus');
let nib = require('nib');

require('gulp-grunt')(gulp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?

let rimraf = require('rimraf');

gulp.task('clean', (cb) => {
rimraf('website/build', cb);
Copy link
Contributor

Choose a reason for hiding this comment

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

Our style is two spaces, not 4.

@crookedneighbor
Copy link
Contributor

This is looking pretty good, just got some style notes for you.

Is there a reason you haven't deleted the other unused tasks for the gruntfile?

@noppanit
Copy link
Author

Sorry I forgot to clean up, I'll do it and push.

@crookedneighbor
Copy link
Contributor

What's the status on this?

@noppanit
Copy link
Author

It's going to be pushed soon

@noppanit
Copy link
Author

The build is failed because

Error: Cannot find module 'xtend'

on module through2 Have you seen this before?

@crookedneighbor
Copy link
Contributor

No. Will look at it this weekend.

@@ -0,0 +1,7 @@
import gulp from 'gulp';

let rimraf = require('rimraf');
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be import rimraf from 'rimraf';

@noppanit
Copy link
Author

Related issue with Travis Failed on CoffeeScript hubotio/generator-hubot#6

@@ -20,3 +54,5 @@ gulp.task('build:dev:watch', ['build:dev'], () => {
gulp.task('build:prod', ['babel:common', 'prepare:staticNewStuff'], (done) => {
gulp.start('grunt-build:prod', done);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

These two extra new lines are not necessary.

@noppanit
Copy link
Author

noppanit commented Nov 6, 2015

@crookedneighbor apologies I'm pretty new at this project. I'm not sure what's causing the wrong css. Can you help checking? I might be that I might have missed something.

@noppanit
Copy link
Author

noppanit commented Nov 6, 2015

After I compare the new css, the positions for sprites are off

@gauthamchandra
Copy link
Contributor

@noppanit, @crookedneighbor I am not too familiar with the codebase but I am with all the technologies and the stack involved. Tell me if you need help with this.

@gauthamchandra
Copy link
Contributor

There needs to be a lot of cleanup with all the npm modules we use. Node is up to v5.0 and many of the modules we use requires v0.10.x and give a ton of deprecated warnings.

Once this PR is merged, I will file a new issue and a matching PR to clean up our dependencies and do a full check to make sure our code is properly linted and follows best practices

@paglias
Copy link
Contributor

paglias commented Nov 8, 2015

@gauthamchandra thanks for help!

A few things to keep in mind, though:

  1. We're going to upgrade to Node v4.2, which is an LTS, not to Node 5. We might review it later but for the time being we'll switch to Node 4.2
  2. We're in the process of upgrading most dependencies, switching to ES6 with ESLint for code style and re-writing the API completely in a separate PR API v3 [WIP] #6144
  3. We'll be happy to accept any PR that upgrades dependencies to the newest version as long as it doesn't break compatibility with API v2 and hasn't already been addressed in API v3 [WIP] #6144

Let me know if you have any question

@noppanit
Copy link
Author

noppanit commented Nov 8, 2015

@gauthamchandra I'm going to merge the conflict so please feel free to jump in this PR. I have two problems

  1. https://github.com/HabitRPG/habitrpg/pull/6112/files#diff-565c7aaf8560ac8a12c341fa07bed43aR60 I can't seem to use

gulp.task('test:prepare:build', ['test:prepare:translations', 'build:dev']); in Travis. The process wouldn't exit I suspect it's because some of the tasks hanging and wouldn't exit and Travis killed the build.

  1. I can't seem to figure out the discrepancy of the cssmin between gulp and grunt. The generated css seems to be broken in gulp

Thanks for your help

@gauthamchandra
Copy link
Contributor

@paglias wow that is a massive set of changes in the new PR. I will forgo the changes and look into that PR instead. 👍

@noppanit
Copy link
Author

@gauthamchandra Sorry I was on the road so didn't have a chance to catch up. Were you able to take a look at the PR?

@gauthamchandra
Copy link
Contributor

@noppanit I got too busy over the weekend and wasn't able to look at it. I will try looking over it in the next 1-2 days.

@noppanit
Copy link
Author

@crookedneighbor I think this PR is not going to work anymore as it's far behind the master. Is it ok if I create a new PR?

@crookedneighbor
Copy link
Contributor

Let's do smaller PRs extracting one grunt task at a time.

@noppanit
Copy link
Author

Will do that.

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

5 participants