remarkjs / gulp-remark

Deprecated Gulp plugin for remark — please use npm scripts and the like
9 stars 3 forks source link

1.0.0 #1

Closed denysdovhan closed 8 years ago

denysdovhan commented 8 years ago

Here we are working on gulp-remark.

denysdovhan commented 8 years ago

cc/ @wooorm @eush77 @iamstarkov

Any suggestions?

wooorm commented 8 years ago

Other than mdastrc(5) and mdastignore(5) support, nope. Looks good! :ok_hand:

denysdovhan commented 8 years ago

@wooorm is there any api how can i pass .mdastrc and .mdastignore configs into mdast?

wooorm commented 8 years ago

You’ll need to integrate with mdast(1) for that, which isn’t very simple I guess.

Best start is the file-set-pipeline, which processes a collection of files. You’ll probably want to use its dependencies (most notably, configure, file-system, and log). There’s a lot going on in mdast(1) so I can imagine you’ll have to take some time to get familiar with the code, but it would be very powerful if gulp-mdast would integrate with the mdast(1), not just with mdast(3).

Before I forget, a lot of plug-ins expose messages, e.g., mdast-lint. It would be very useful to have gulp-mdast expose those messages somewhere (either using the aforementioned log module or using vfile-reporter directly).

P.S. mdast(1) == CLI, mdast(3) == API.

denysdovhan commented 8 years ago

Is there possible way to require it from mdast?

For examle, how it was done in gulp-eslint:

var CLIEngine = require('eslint').CLIEngine;
…
var linter = new CLIEngine(options);

Maybe could you create api for these cases (I mean grunt plugin, webpack loader e.g)? Because it seems to be a bit difficult.

wooorm commented 8 years ago

Of course it’s require-able: require('mdast/lib/cli'), for example, would work just fine!

As mentioned earlier, this is indeed a bit advanced. There are no external documents to guide you. Don’t let that discourage you, though. There are a lot of docs inline. And, to be fair, there’s also a lot of complexity going on, which is the reason why it might seem complex :wink:

I’d appreciate it if you’d give it a go, but don’t fret to ask me if you run into things. Also, I may have some time tomorrow to try to whip something up! :smile:

denysdovhan commented 8 years ago

Oh, thank you. Have you got invite into this repo?

wooorm commented 8 years ago

Yup, thanks :+1:

denysdovhan commented 8 years ago

Ok. Will dive up into sources.

denysdovhan commented 8 years ago

There is a problem. I noticed it just now. gulp-mdast is already on npm.

What should we do?

wooorm commented 8 years ago

Seems @shinnn isn’t maintaining it (am I correct, Shinnosuke?) In which case, this project should be able to continue from version 0.0.4.

wooorm commented 8 years ago

Just saw shinnn/gulp-mdast was published today, just not on GH yet. Meaning, the same work happens in two places, which is generally a bad idea :smile:

shinnn commented 8 years ago

@denysdovhan @wooorm

It's OK to add @denysdovhan to the npm owners. I guess @denysdovhan can update gulp-mdast project more frequently because I'm already maintaining lots of other OSS projects.

wooorm commented 8 years ago

Thanks, @shinnn. I hope that’s the best for gulp-mdast :smile: Any suggestions you came across when creating shinnn/gulp-mdast we should keep in mind?

P.S. Just checked npm; am I right that you haven’t added @denysdovhan yet? No haste, just making sure you haven’t forgotten :wink:

shinnn commented 8 years ago

Just checked npm; am I right that you haven’t added @denysdovhan yet?

@wooorm, I just added @denysdovhan and you.

wooorm commented 8 years ago

:ok_hand:

denysdovhan commented 8 years ago

So will we continue in my repo or wherever e

wooorm commented 8 years ago

@denysdovhan Well, this repo is on GH. So I’d say here. Just ensure, when publishing, you start at 0.0.4.

denysdovhan commented 8 years ago

I'd say do it here too. @shinnn, do you want to be added into this repo?

… you start at 0.0.4.

Maybe better to start from 0.1.0 or even 1.0.0. We're changing API using gulpMdast().use() method, therefore, according to semver, we should bump major version. What do you think?

iamstarkov commented 8 years ago

semver doesnt dictate to do anything until 1.0

iamstarkov commented 8 years ago

I would say 0.1 is good enough to test API and find obvious bugs

wooorm commented 8 years ago

@denysdovhan I completely agree regarding semver, I was responding to the fact that there are two separate gulp-mdast’s, but that won’t pose a problem if all versions used by denysdovhan/gulp-mdast are higher than shinnn/gulp-mdast. :wink:

denysdovhan commented 8 years ago

ok. I'll continue from 0.1

shinnn commented 8 years ago

https://github.com/denysdovhan/gulp-mdast/pull/1#issuecomment-162631153

do you want to be added into this repo?

Of course! Thanks :)

denysdovhan commented 8 years ago

@shinnn You're invited. Sorry for late. I'm a little busy these days

denysdovhan commented 8 years ago

I'm not sure how should API look like.

gulpRemark([options]) and attaching plugins through gulpRemark.use(plugin[, options]) — is it good API?

denysdovhan commented 8 years ago

@wooorm can I somehow call .use(plugin) and pass this FileSet into .transform(context)? Or pushing plugins into context.plugins it's the only way?

wooorm commented 8 years ago

@denysdovhan Nope, the use function on file-set is only applicable for file-sets and completer functions, not for normal plug-ins.

Currently, it’s not possible to inject functions to be used as plug-ins on the CLI, only strings on the context object are injected into the from .remarkrc files detected plugins object.

If, in the future, we’re planning on allowing the injection of plug-ins, it should be somewhere close to here I think.

denysdovhan commented 8 years ago

@wooorm as I understood, we can pass plugins only like this:

gulp.task("default", () =>
  gulp.src('*.md')
    .pipe(gulpRemark({
      plugins: {
        "lint": {
          "code-block-style": false
        }
      }
    }))
);

but it looks terrible. I wanna make something like this:

gulp.task("default", () =>
  gulp.src('*.md')
    .pipe(gulpRemark().use(lint, { "code-block-style": false }))
);

It's better, isn't it? Any idea how can I implement this?


Currently, it’s not possible to inject functions to be used as plug-ins on the CLI

If there isn't possible way to do this, maybe should we make it possible before?

wooorm commented 8 years ago

@denysdovhan I’ve added support for injecting plugins into remark(3) from remark(1). Read more about it: https://github.com/wooorm/remark/commit/4e65d7086041240047991c3f459e21811983978a.

Released in remark@3.1.0.

denysdovhan commented 8 years ago

@wooorm You have an mistake in https://github.com/wooorm/remark/commit/4e65d7086041240047991c3f459e21811983978a commit message:

var CLI = require('remark/lib/cli/cli');
var engine = require('remark/lib/cli');
var html = require('remark-html');

var cli = new CLI({
    'files': 'readme.md'
}).use(html);

run(cli, function (err, success) {
    console.log('done!', err, 'successfully?', success);
});

You required engine, but use run() which isn't required. By the way, remark/lib/cli doesn't export run() function. Or maybe I'm wrong?

This makes me confused, so I still don't understand how may I use it.

wooorm commented 8 years ago

I changed the name of engine halfway to run, which in this case made more sense. Just replace both with something else, e.g., whoops, and stuff will work ;)

denysdovhan commented 8 years ago

Okay, got it)

If understood correctly, now I can not to use configure() and transform(). That means I can just pass context into run(), right?

wooorm commented 8 years ago

Close. The example in the commit wasn’t meant as being a replacement for this project though. What that commit does, is add support for inject plugins (as functions) into the CLI.

The following code is now supported:

var cli = new CLI({
    'files': 'readme.md'
}).use(html);

This means you could a bit of the code in gulp-remark, specifically, L28-L36:

    context = {
      detectRC: true,
      cwd: process.cwd(),
      stdout: process.stdout,
      stderr: process.stderr,
      files: [
        vFile
      ]
    };

...with something like the first example.

E.g., the following:

var cli = new CLI({
  detectRC: true,
});

cli.files = [vFile];
cli.use(plugin);

Good luck!

denysdovhan commented 8 years ago

Thank you, I think I've got it.

denysdovhan commented 8 years ago

Hohoho!

It's almost done! Works with .remarkignore and .remarkrc, prints linter's reports and processes files!

image

Now, I'm gonna write some tests and new readme. Does anybody have any suggestions?

UPD: Woops. Exactly it still doesn't read ignore files :(

denysdovhan commented 8 years ago

@wooorm one more question. If I understood correctly, remark/lib/cli/ignore.js just read folder and ignore files using .remarkignore. Am I right?

If I am, please, can you add checking by VFile.filePath()?

I ask you about it, because cli.traverser actually contains Finder { ignore: [ 'README.md', 'node_modules/' ], … }, but it's doesn't ignore anything:

image

Possibly it happens 'cause gulp load every file that match with pattern defined in gulp.src() (currently *.md), but remark does not ignore them.

wooorm commented 8 years ago

Which code produces the output shown in the image? The current last commit, or are there specific changes?

denysdovhan commented 8 years ago

I've just added:

console.log(cli.traverser);

before L36 and change .remarkignore like this:

- example.md
+ README.md
denysdovhan commented 8 years ago

@wooorm oh, and I've change context:

  const cli = new CLI({
    detectRC: true,
    detectIgnore: true,
    cwd:      process.cwd(),
    stdout:   process.stdout,
    stderr:   process.stderr
  });
wooorm commented 8 years ago

Unfortunately, it’s not possible to ignore inside the CLI, as the ignore is only used when searching for files by globs. In Gulp, you’re not searching for files, thus the ignore is never reached. There are also some other good reasons why this shouldn’t change, which I won’t dig into.

As a solution, however, you are able to use cli.traverser.ignore.shouldIgnore(filePath) inside gulp-remark to check whether the filePath of a file is inside the ignored patterns, since remark@3.2.2. Hope that helps!

denysdovhan commented 8 years ago

Ok, I'm trying to make it works. I wanna clarify: does cli.traverser.ignore.shouldIgnore(filePath) compare with basename or with absolute name?

denysdovhan commented 8 years ago

I've sent pull request to vFile with added vFile#fileBasename method. @wooorm please take a look.

denysdovhan commented 8 years ago

OK, I've fixed tests and everything works.

But we have another problem. How can I pass into cli remark options? I've already seen this, but definitely that's not what I'm looking for.

wooorm commented 8 years ago

Currently, that’s possible through settings, which expects a string.

To be honest, maybe we should first push 1.0.0 now, and add this later. I don’t think it’s really blocking, because it’s possible to specify settings through .remarkrc files now.

Do you agree?

denysdovhan commented 8 years ago

Okay, so I'm gonna write some tests and new README.md.

Do you wanna release 1.0.0 or 0.1.0?

wooorm commented 8 years ago

1.0.0, as it’s also higher than the current.

denysdovhan commented 8 years ago

Oh, right. Okay, I hope I'll finish today.

shinnn commented 8 years ago

@andrepolischuk @denysdovhan @wooorm Can you give me write access to gulp-remark package?

Also, what is this repo? Current npm page links to it.

denysdovhan commented 8 years ago

@shinnn it's a @andrepolischuk ’s repo. I've already sent you invite to this repo. If you're talking about npm package, you was primary owner.

UPD: Added you to npm owners.