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

Getting Started documentation #2182

Merged
merged 12 commits into from
Jul 29, 2018
Merged

Getting Started documentation #2182

merged 12 commits into from
Jul 29, 2018

Conversation

phated
Copy link
Member

@phated phated commented May 31, 2018

This is the beginning of our full documentation rewrite. It is meant to be the "Getting Started" section on the upcoming docs website with a target audience of:

  • JS developers that haven't written gulp
  • People new to both JS and gulp

It is meant to be read in numerical order (the filenames start with 1,2,3,etc).

Any feedback is very much welcomed and appreciated.

cc @stevelacy @contra @terinjokes @jonschlinkert @doowb @callumacrae @demurgos

@phated
Copy link
Member Author

phated commented May 31, 2018

Also posing an open question about whether or not to include documentation/usage of pump() for these "intro" docs and examples. It felt like it would add a bunch of complexity.

if (file.isBuffer()) {
const code = uglify.minify(file.contents.toString())
file.contents = Buffer.from(code)
}
Copy link

Choose a reason for hiding this comment

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

@phated, this is probably missing cb(null, file) if I'm not mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, missed that.

@backflip
Copy link

backflip commented Jun 1, 2018

Also posing an open question about whether or not to include documentation/usage of pump() for these "intro" docs and examples. It felt like it would add a bunch of complexity.

To me this would mostly depend on whether you consider using pump() to be a reasonable default approach. In general, an intro to error handling would definitely make sense to me.

Anyway, great rewrite so far! Would you appreciate help on specific topics now?

```

### Verify your gulp versions
```sh
Copy link

Choose a reason for hiding this comment

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

This would probably need some instructions on what to do with output.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the assumption was that the output should look like the screenshot otherwise you need to do the steps again but maybe that's a poor assumption to make.

);
```

When a composed operation is run, each task will be executed every time it was referenced. For example, a `clean` task referenced before two different tasks would be run twice and lead to undesired results. Tasks can be wrapped with the [async-once][async-once] module if this **(not recommended)** pattern is needed.
Copy link

Choose a reason for hiding this comment

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

Is there a recommended pattern for the same problem? Otherwise I think this note isn't very helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a pattern that people migrating from 3.x might encounter so I thought it was worth mentioning that they can use async-once but the "right way" to do it would be to restructure your tasks so the task was only specified once - the pattern recommended throughout the document.

I'm open to adjusting the language but I think this is worthwhile enough to keep.

@phated
Copy link
Member Author

phated commented Jun 1, 2018

To me this would mostly depend on whether you consider using pump() to be a reasonable default approach.

It should be the default going forward (and switched to stream.pipeline from node core once 10 becomes LTS and 8 rotates out).

In general, an intro to error handling would definitely make sense to me.

I like this idea. I'll discuss with @janiceilene

@phated
Copy link
Member Author

phated commented Jun 9, 2018

Ping - still looking for y'all to review.

@stephenlacy
Copy link
Contributor

I did a review of the code vs documentation blocks and did not notice any issues that were not mentioned before. Do we want to show an example of a synchronous task as well as stating that it should not be used?

@stephenlacy stephenlacy mentioned this pull request Jul 10, 2018
@phated
Copy link
Member Author

phated commented Jul 29, 2018

Alright all, I've done a final pass and caught some things that I didn't like. I've also made the fixes suggested so I'm going to get this merged.

@phated phated merged commit 2bd75d0 into master Jul 29, 2018
@phated phated mentioned this pull request Jul 29, 2018
@phated phated deleted the getting-started branch August 21, 2018 23:18
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.

8 participants