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

Trailing spaces in gulp --tasks output breaks WebStorm tooling. #24

Closed
mikehaas763 opened this issue Apr 17, 2015 · 26 comments
Closed

Trailing spaces in gulp --tasks output breaks WebStorm tooling. #24

mikehaas763 opened this issue Apr 17, 2015 · 26 comments

Comments

@mikehaas763
Copy link

Right now, WebStorm parses the output of gulp --tasks to capture the tasks that can be run. I'm testing this with the v4 alpha of both gulp and gulp-cli. I have a very simple gulpfile with a single task. I've noticed that the output from running gulp --tasks now has two spaces appended to my task name. So when WebStorm parses the task tree, it attempts to run gulp "clean " which does not exist (notice the two trailing spaces).

I've already submitted a bug to Jetbrains letting them know that they should probably trim the task names after parsing them. I see a couple of things that could be improved on the gulp side too. I think the first and most obvious step is to remove the trailing spaces from the task tree output. I have a couple of other ideas also.

I know gulp uses Liftoff for CLI work so maybe it can't be easily addressed directly in the gulp project, but I'm thinking the gulp-cli should trim the tasks arguments that are passed in just as a simple normalization step. I don't think anyone should be defining tasks with whitespace on either side so this shouldn't cause a problem. That would create a simple fix for any other tooling out there that parses the task tree and does not do its own trim. This is to keep existing tooling happy and not cause any backward compatibility issues.

It could be taken one step further though and I may be interested in spending some time implementing it. There should be a separate CLI flag that can be used by other tooling that outputs to something that does not need to be manually parsed. I'm thinking a simple object graph version of the tree serialized to JSON and written to stdout. This should help to make any tooling integration with gulp easier.

Update:
I just noticed that gulp is using archy, which means this object is probably already defined somewhere.

@yocontra yocontra added the bug label Apr 17, 2015
@mikehaas763
Copy link
Author

I just got feedback from Jetbrains. Apparently this was already brought up and WebStorm will be using --tasks-json (which I just discovered in another issue) as of 10.0.2

Source here:
https://youtrack.jetbrains.com/issue/WEB-16000

@mikehaas763
Copy link
Author

Should probably leave this issue open because in any case I don't think the regular tree output should be appending spaces to ensure backward compatibility with older version of WebStorm and other tooling. Agreed?

@phated
Copy link
Member

phated commented Apr 17, 2015

This is a side-effect of us supporting task descriptions now. We've exposed the entire task tree for use by tooling through the --tasks-json CLI flag and the tree API (recently documented at https://github.com/gulpjs/gulp/blob/4.0/docs/API.md#gulptreeoptions). It looks like we are missing docs for --tasks-json but I will open an issue now.

Btw, do you have any idea how to comment on the jetbrains issue tracker? I would highly suggest they use the gulp.tree API instead of CLI if at all possible.

@phated phated closed this as completed Apr 17, 2015
@mikehaas763
Copy link
Author

@phated Ya I've been scouring issues and docs today relating to gulp/gulp-cli and undertaker so I eventually figured that out.

Task descriptions: Ah, I see. So I'm guessing the resulting two spaces is a side effect of a printf()-like functionality where you set a specific number of character aside for a column?

Commenting on jetbrains issue: Yes, I think you just have to log in at the top right.

The IntelliJ IDE platform is not a node app, so they can't simply require('gulp') for interoperability. How do you propose they interop with gulp.tree? From what I can see they would have to essentially wrap the gulp.tree invocation with their own module (which would need to be embedded with the IDE program files). That module would then essentially just get the resulting object from invoking gulp.tree() and expose it in some form that the IDE can consume. Likely, by outputting to stdout.

@phated
Copy link
Member

phated commented Apr 17, 2015

@mikehaas763 yeah, the issue comes from pretty-printing columns for the descriptions.

I think they could create a tiny bin file that expects to be run in node. Reading through the comments on that thread, it seems like it would be less hacky than shelling out to gulp and parsing the output, looking for [ and ]. Maybe we can switch --tasks-json to do a simple print like --tasks-simple. If you have contacts at jetbrains, I'd love for them to join in on this convo because I want to make tooling interop as nice as possible.

@mikehaas763
Copy link
Author

@phated I don't have any contacts there but I'm fairly positive that @segrey who opened gulpjs/gulp#584 is the same Sergey that is responding to the Jetbrains issue ticket.

@segrey
Copy link

segrey commented Apr 20, 2015

Hi guys,
Thanks for the invitation. Glad to join this conversation.
To fetch tasks for gulp v3 WebStorm uses --tasks cli option.
For gulp v4 --tasks-json is being used. I'm pretty happy with the json output so far. The only thing is that json occupies several lines and if gulpfile outputs anything to stdout during the task registration phase, that might complicate interop a bit. A tiny node bin would have the same problem I think.
Btw, WebStorm grunt integration has its own node bin that uses grunt API. It was implemented in this way, because grunt doesn't provide a cli option to output tasks without executing a default task.
Probably, output format of --tasks-json could be improved somehow to guarantee smooth interop. Otherwise (in case of tiny node bin on a tool side), each tool should solve the same problem independently.

@mikehaas763
Copy link
Author

@segrey I think what he's saying is that you would call from the node bin "gulp.tree()". Then you would be in control of what can get logged to stdout. Is that right @phated?

I do like the idea of being able to call directly into code to get an object but what is the purpose of --tasks-json if it cannot be guaranteed a whole and valid JSON document? In the case of someone logging during the task registration phase as he mentioned.

@yocontra
Copy link
Member

Could noop process.stdout if --tasks or --tasks-json was passed to the CLI to make sure nothing but the tasks gets logged

@segrey
Copy link

segrey commented Apr 21, 2015

@contra That's a solution. Other possible solutions:

  • reformat JSON to make it occupy one line
  • print JSON to a file specified by a new cli option

@phated
Copy link
Member

phated commented Apr 21, 2015

I like the idea of using JSON.stringify to reduce the JSON to a single output line. @contra thoughts?

@yocontra
Copy link
Member

@phated I think nooping stdout/stderr is the more robust solution because there are multiple tools (for example, all of our autocomplete script) that rely on this output being clean and usable

@phated
Copy link
Member

phated commented Apr 21, 2015

@contra I understand that sentiment but it just seems so dirty to hijack stdout. I actually rely on the ability to console.log whenever while creating gulpfiles.

@yocontra
Copy link
Member

@phated only when --tasks or --tasks-json is passed, not like it would always hijack it

@mikehaas763
Copy link
Author

I agree that it seems dirty to hijack it though it doesn't seem so bad if limited to only specific routines where it's needed like --tasks-json. I'd think it would be worse to have someones tooling break because they forgot a console.log somewhere than it would be to suppress their console.log for routines that are intended for tooling.

Heh, if it ever became a problem you could extend --tasks-json with an optional flag like --enable-logging or --enable-stdout.

@segrey
Copy link

segrey commented Apr 22, 2015

Nooping stdout sounds reasonable. What bothers me a bit about nooping stdout is that inserting console.log in gulpfile is a way to find out the cause of not loaded task list in IDE. Another way is to start a debug session attached to gulp --tasks-json process, but that requires more effort from a user.

About nooping stderr: wouldn't it hide exceptions raised in gulpfile? Probably, nooping stderr is not needed because tools can work with standard streams independently.

@phated
Copy link
Member

phated commented Jul 12, 2015

Reopening this because I almost lost it and forgot about it. Can we come up with the best solution for this?

@segrey
Copy link

segrey commented Jul 13, 2015

IMO, the most reliable solution is to dump JSON to a file specified by some cli option, like gulp --tasks-json --output=tasks.json. Behavior is clear and predictable. But, it requires introducing a new cli option. So, I'm not sure it is worth. Nooping stdout is also OK for me, but please don't noop stderr, it might contain important message (moreover tools are able to handle stdout only).

@phated
Copy link
Member

phated commented Jul 19, 2015

We can probably use https://github.com/isaacs/mute-stream to mute the stream during load, unmute to spit out the json and then mute again until the process finishes.

@segrey why is a JSON file the best option? We could probably add an optional parameter to --tasks-json (e.g. --tasks-json=tasks.json) but I'd like to know why that is the best option.

@segrey
Copy link

segrey commented Jul 31, 2015

@phated I think nooping stdout stream is a good idea. However, JSON file is a bit clearer for me, because it exploits a clear way of separating streams. BTW, I couldn't mute stdout stream using this:

var MuteStream = require('mute-stream')
var ms = new MuteStream()

ms.pipe(process.stdout)
ms.mute()
process.stdout = ms
ms.write('Muted output\n')
process.stdout.write('process.stdout.write\n')

var fs = require('fs')
fs.write(process.stdout.fd, 'fs.write\n')
fs.writeSync(process.stdout.fd, 'fs.writeSync\n')
console.log('console.log')

Output (node 0.12.7):

process.stdout.write
fs.write
fs.writeSync
console.log

@phated
Copy link
Member

phated commented Aug 21, 2015

@segrey I just pushed a change that allows an output file to be specified for the --tasks-json argument. I also removed the pretty printing because this option is not made for human consumption.

Example usage:
gulp --tasks-json tasks.json

@segrey
Copy link

segrey commented Aug 21, 2015

Cool, thank you.

phated added a commit that referenced this issue Aug 21, 2015
@phated
Copy link
Member

phated commented Aug 21, 2015

Just added support for muting the process.stdout while requiring a gulpfile if we see a task listing flag. Closing, as I believe both use cases are handled now.

@phated phated closed this as completed Aug 21, 2015
@YuriSizov
Copy link

Hello! I'd like to point out that this check in lib/versioned/^4.0.0-alpha.1/index.js probably doesn't work as intended

if (typeof opts.tasksJson === 'boolean' && opts.tasksJson) {
  return console.log(output);
} else {
  return fs.writeFileSync(opts.tasksJson, output, 'utf-8');
}

As mentioned before, WebStorm uses --tasks-json option, and for me this very commit broke integrated gulp tool in WS. If used as a boolean (while CLI options are marking it as a string), it is skipped by yargs, as it detects this option as an empty string. For obvious reasons it works, if you use --tasksJson, but I don't think it's possible to change that command in WebStorm 10 as a user.

I am not sure if WebStorm is going to change in future to accommodate this commit, but please, if it's possible, reopen this issue to find a better, non-breaking solution.

@phated
Copy link
Member

phated commented Sep 29, 2015

@pycbouh fyi, you should be opening new issues for new issues, not commenting on extremely old issues. I did it for you 😒

@YuriSizov
Copy link

@phated Yeah, sorry :( I wanted to continue this discussion, as this is where the problem had been created, but didn't realize the right way would've been to create a new issue and then link this one there.

Thanks for educating me and fixing the problem anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants