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

Force gulp.src to fix order of files #687

Closed
fesor opened this issue Sep 16, 2014 · 36 comments
Closed

Force gulp.src to fix order of files #687

fesor opened this issue Sep 16, 2014 · 36 comments

Comments

@fesor
Copy link

fesor commented Sep 16, 2014

If i just pass mainBowerFiles result, which ordered based on dependencies for each module, into gulp's stream, then all files will be reordered and it broke app for example for inject or concat.

For example, mainBowerFiles returns:

[
    '/some/path/to/src/lib/angular/angular.js',
    '/some/path/to/src/lib/angular-animate/angular-animate.js',
    '/some/path/to/src/lib/angular-sanitize/angular-sanitize.js',
    '/some/path/to/src/lib/angular-ui-router/release/angular-ui-router.js',
]

after gulp-inject we'll get:

<script src="scripts/angular-animate.js"></script>
<script src="scripts/angular-sanitize.js"></script>
<script src="scripts/angular-ui-router.js"></script>
<script src="scripts/angular.js"></script>

Since ur-router, angular-sanitize and angular-animate depends of angular, application crashes.

Can this be solved?

@sindresorhus
Copy link
Contributor

gulp.src reads the files in order, but plugins are not ordered as it's beneficial for files to not block each other through the pipeline for maximum performance.

You can use a plugin like https://github.com/sirlantis/gulp-order to order the file. But depending on file order is usually an anti-pattern anyways.

@yairEO
Copy link

yairEO commented Nov 25, 2014

I have just updated gulp the latest version (5 minutes ago) and files order definitely changed.

This simple task just creates random code instead of filename-ordered concatenated code.

gulp.src('./js/*.js')
    .pipe(concat('_all.js'))
    .pipe(gulp.dest('./js/'));

Really.. this happens right after I ran npm install gulp. So I think it's something wrong with the gulp src function which generates the stream.

Update

I had to revert gulp back to (older) version 3.5 until this issue is resolved.
Please open the issue...Thanks!

@heikki
Copy link
Contributor

heikki commented Nov 25, 2014

@yairEO Minimal example which reproduces the issue would be helpful.

@yairEO
Copy link

yairEO commented Nov 25, 2014

you are absolutely right. I will produce a test now (but this should be automatically tested by a test suit..)

@yairEO
Copy link

yairEO commented Nov 25, 2014

Here, a test project that shows the bug:

https://github.com/yairEO/gulp-src-test/

@heikki
Copy link
Contributor

heikki commented Nov 25, 2014

I get the expected (a, b, c) order on osx 10.10.1.

ps. You should remove node_modules & put it to .gitignore. Add package.json instead.

@yairEO
Copy link

yairEO commented Nov 25, 2014

yes you are right i should have put just package.json file.
Anyway, it does happen to me on windows 7 and it never happened before with other gulp version. This bug started recently, and it is totally confirmed by the test case.

@heikki
Copy link
Contributor

heikki commented Nov 26, 2014

I'm seeing the wrong order (c, a, b) on VirtualBox windows 7. It started happening with gulp 3.7.0.

module diff
gulp 3.6.2 <-> 3.7.0
vinyl-fs 0.1.4 <-> 0.2.1
graceful-fs 2.0.3 <-> 3.0.4

I have no clue why but downgrading graceful-fs to 2.0.3 fixes it.

--edit

Narrowed down to this commit: isaacs/node-graceful-fs@08471b2

@yairEO
Copy link

yairEO commented Nov 26, 2014

this is a major major bug..someone should re-open this issue.

I absolutely love Gulp and am using it in all my projects and this caused quite a mess

@heikki
Copy link
Contributor

heikki commented Nov 26, 2014

It looks like node-glob doesn't emit match events in sorted order. glob-stream listens those events and older version of graceful-fs has masked this issue somehow.

I guess it's up to @contra to decide if and where this should be handled.

@heikki
Copy link
Contributor

heikki commented Nov 27, 2014

I have a test & fix here: https://github.com/heikki/glob-stream/compare/master...fix-order

  • Test would show the problem on windows if mocha didn't use an old version of glob that in turn uses old version of graceful-fs 🙀
  • Here's benchmark that shows why the fix is no good (streaming starts a lot later)

I think the original cause is that graceful-fs readdir (or something) is sorted. When monkey patching the global was changed to eval then behaviour changed to normal where graceful-fs wasn't directly used. Normal for windows is different order.

@yocontra yocontra reopened this Nov 27, 2014
@yocontra
Copy link
Member

@heikki That fix will mess up the speed of gulp. One of the great things is that glob matches get emitted immediately, not after the whole fs has been traversed. Has anyone opened an issue on graceful-fs that they broke something here?

@heikki
Copy link
Contributor

heikki commented Nov 27, 2014

I haven't opened any issues yet. Maybe the right place to fix this would be in node-glob? There's no need to sort all the matches at once. Just sort the result of each readdir (or however it works) like it happened when graceful-fs was messing things.

@yocontra
Copy link
Member

@heikki Yeah it should go on glob if they introduced a regression without realizing it

@heikki
Copy link
Contributor

heikki commented Nov 30, 2014

Opened issue in node-glob. ^

@heikki
Copy link
Contributor

heikki commented Nov 30, 2014

Works now.

@yocontra
Copy link
Member

Closing, an npm update should fix this

@mbcooper
Copy link

mbcooper commented Dec 9, 2014

@contra
I see this as closed. However, i have all the latest and still have files coming back in odd orders ...

@yocontra
Copy link
Member

yocontra commented Dec 9, 2014

@mbcooper Run npm cache rm && npm update and if it still isn't working run npm ls glob and put the results here

@heikki
Copy link
Contributor

heikki commented Jan 13, 2015

FYI, fix has been reverted in node-glob. isaacs/node-glob#143 (comment)

@heikki
Copy link
Contributor

heikki commented Jan 13, 2015

Hm, what now? Should we leave this as is & add docs or do something in glob-stream if sort-option is specified?

@phated
Copy link
Member

phated commented Jan 13, 2015

What about a config option that globs all before beginning to emit?

@heikki
Copy link
Contributor

heikki commented Jan 13, 2015

Sync mode results are sorted by default so this ugly tweak probably works:

if (opt.sync) {
  globber.found.forEach(function(filename) {
    stream.write({
      cwd: opt.cwd,
      base: basePath,
      path: path.resolve(opt.cwd, filename)
    });
  });
  stream.end();
} else {
  globber.on('error', stream.emit.bind(stream, 'error'));
  globber.on('end', function(/* some args here so can't use bind directly */){
    stream.end();
  });
  globber.on('match', function(filename) {
    stream.write({
      cwd: opt.cwd,
      base: basePath,
      path: path.resolve(opt.cwd, filename)
    });
  });
}

https://github.com/wearefractal/glob-stream/blob/58358ff102e2c5b379ec2f21c938f4b9706385ba/index.js#L29-L39

@yocontra
Copy link
Member

I think the cleaner way to do this is to let people specify a sort option, and if given it will tell glob-stream to pipe itself through a stream sorter

@yocontra yocontra reopened this Jan 13, 2015
@phated
Copy link
Member

phated commented Jan 14, 2015

I like async ordering to belonging in userland. Why build it in core?

@yocontra
Copy link
Member

@phated What plugin do you use?

@phated
Copy link
Member

phated commented Jan 14, 2015

@contra I don't use one, but https://github.com/sirlantis/gulp-order doesn't look bad. I think it should be updated to through2, but no other gripes really.

@yocontra
Copy link
Member

@phated Doesn't seem to support the features needed to solve this specific ordering problem (alphabetical)

@phated
Copy link
Member

phated commented Jan 14, 2015

@contra it can be adapted. I think userland should have a gulp-reorder that accepts a sort function.

@yocontra
Copy link
Member

@phated
Copy link
Member

phated commented Jan 15, 2015

@contra https://www.npmjs.com/package/sort-stream2 should work with a custom sort function

@heikki
Copy link
Contributor

heikki commented Feb 12, 2015

Depending on the use case use either gulp-sort (alphabetical etc.) or gulp-order (globs)

@heikki heikki closed this as completed Feb 12, 2015
@nicholas-johnson
Copy link

I'd just interject that non-deterministic ordering causes unnecessary conflicts when managing a deploy via Git. It's cool, yet painful.

@musterknabe
Copy link

Why not let the user decide via flag if gulp.src should return reliably ordered results (slower) or random results (faster)?

@mattsawyer77
Copy link

totally agree with @musterknabe.

@vniche
Copy link

vniche commented Dec 17, 2015

@musterknabe +1

@gulpjs gulpjs locked and limited conversation to collaborators Dec 17, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests