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

fix build/watch for people importing tasks into their own gulpfile #2668

Merged
merged 1 commit into from
Jul 22, 2015

Conversation

fholzer
Copy link

@fholzer fholzer commented Jul 19, 2015

See #2653

@fholzer
Copy link
Author

fholzer commented Jul 19, 2015

This way, the gulp instance that is handed down to rtl.js supports gulp-help, as it adds new tasks to gulp.

@fholzer
Copy link
Author

fholzer commented Jul 20, 2015

In case you're worried about idempotence of gulp-help, I tested it. You could do this

var gulp = require('gulp-help')(require('gulp-help')(require('gulp-help')(require('gulp'))));

gulp.task('default', 'this is some default task', false, function(done) {
        console.log('some task output');
        done();
});

and still get the expected output

storm@linux-krxl:~/git/semanti-ui/patch/test> gulp 
[09:22:29] Using gulpfile ~/git/semanti-ui/patch/test/gulpfile.js
[09:22:29] Starting 'default'...
some task output
[09:22:29] Finished 'default' after 107 μs
storm@linux-krxl:~/git/semanti-ui/patch/test> gulp help
[09:26:40] Using gulpfile ~/git/semanti-ui/patch/test/gulpfile.js
[09:26:40] Starting 'help'...

Usage
  gulp [TASK] [OPTIONS...]

Available tasks
  default  this is some default task
  help     Display this help text.

[09:26:40] Finished 'help' after 978 μs
storm@linux-krxl:~/git/semanti-ui/patch/test>

@fholzer
Copy link
Author

fholzer commented Jul 20, 2015

Edit: This is still required.

@fholzer fholzer closed this Jul 20, 2015
@fholzer fholzer reopened this Jul 20, 2015
@jlukic
Copy link
Member

jlukic commented Jul 20, 2015

How are you importing tasks, with two parameters or three?

I think it might be a bit idiosyncratic to require other people who import the tasks to use gulp-help

fholzer added a commit to fholzer/Semantic-UI-Issue-Showcase that referenced this pull request Jul 21, 2015
@fholzer
Copy link
Author

fholzer commented Jul 21, 2015

The alternative is to remove the gulp-help argument from gulp.task from all files that get sourced by tasks/build.js and tasks/watch.js. That would be nearly all tasks as can be seen when running gulp help in my fixed version:

gulp help
[12:31:11] Using gulpfile ~\Documents\git\semantic-ui-tests\gulpfile.js
[12:31:11] Starting 'help'...

Usage
  gulp [TASK] [OPTIONS...]

Available tasks
  build
  build-assets                   Copies all assets from source
  build-css                      Builds all css from source
  build-javascript               Builds all javascript from source
  build-rtl                      Watch files as RTL
  default
  help                           Display this help text.
  package compressed css
  package compressed docs css
  package compressed docs js
  package compressed js
  package compressed rtl css
  package uncompressed css
  package uncompressed docs css
  package uncompressed docs js
  package uncompressed js
  package uncompressed rtl css
  watch
  watch-rtl                      Build all files as RTL

[12:31:11] Finished 'help' after 2.26 ms

As these come from files that are also sourced when running gulp from the semantic directory, removing the gulp-help argument from them would break running gulp help in the semantic/ directory.
While keeping gulp-help around, and fixing the issue for users not directly using gulp-help in their gulpfile.js is just a matter of editing two files, and gulp help would still show all tasks when run from the semantic/ directory.

The nice thing about my PR is that users aren't forced to use gulp-help in their gulp file, if they don't want to. Though even when they to, it will still work.

Negative side-effect: When users use gulp-help in their project, they also see all the semantic targets.
Though in order to prevent that, users shouldn't require() the semantic task files at the top of their gulpfile.js, but only in the task function that actually runs those tasks, like so. (Of course this also works file for people that don't use gulp-help.)

@jlukic
Copy link
Member

jlukic commented Jul 22, 2015

I'll be evaluating this shortly.. hold tight

@jlukic jlukic added this to the 2.1 milestone Jul 22, 2015
@jlukic
Copy link
Member

jlukic commented Jul 22, 2015

I've tested this and it works as you say, using either gulp help or no gulp help in the imported gulpfile.js now works fine.

jlukic added a commit that referenced this pull request Jul 22, 2015
fix build/watch for people importing tasks into their own gulpfile
@jlukic jlukic merged commit d87fa69 into Semantic-Org:next Jul 22, 2015
@jlukic
Copy link
Member

jlukic commented Jul 22, 2015

I've created an example repository which I will include in docs to help people understand what to do.
https://github.com/Semantic-Org/Example-External-Gulpfile

@fholzer fholzer deleted the patch-1 branch July 22, 2015 20:49
@jlukic jlukic modified the milestones: 2.1, 2.0.7 Jul 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants