sindresorhus / gulp-changed

Only pass through changed files
MIT License
742 stars 44 forks source link

Do not attempt comparing SHA1 for directories #65

Closed chris54721 closed 7 years ago

chris54721 commented 7 years ago

compareSha1Digest does not check whether sourceFile represents a directory, which can happen for example when using glob patterns for src. This PR makes sure only files are processed; at the moment if the source paths contain a directory the following error is thrown:

(node:9418) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error in plugin 'gulp-changed'
Message:
    EISDIR: illegal operation on a directory, read
Details:
    errno: -21
    code: EISDIR
    syscall: read
    fileName: [directory path]
sindresorhus commented 7 years ago

You need to rebase on master since I just landed #64.

@generalov Any thoughts on this?

chris54721 commented 7 years ago

Well that was bad timing.

generalov commented 7 years ago

Very interesting, guys! In the ideal, any promise rejects should be handled. Could you please add some unit tests for more details to describe this case?

I've nether come across this issue in our project. It seems the issue is avoided by the gulp-if in the pipe: .pipe($.if('**/*.css', cssChannel(), $.gulp.dest(destPath))) So just files are passed to the cssChannel substream where the gulp-changed actually is used.

generalov commented 7 years ago

That's the expected behavior for the directories?

  1. They should be passed to the output stream without any change detection.
  2. They should be excluded from the output stream by the gulp-changed.
  3. They shouldn't be occurred in the input stream.
  4. The some implementation should be suggested to compare directories by a content?
  5. Something else?

The 1 and 2 could be solved by the appropriate error handling in the ".catch" function. The 3 by updating the documentation with may be a gulp-if example. The 4 is required the investigation.

chris54721 commented 7 years ago

I think the best solution would be pushing directories to the stream only if there are some file changes in them, since it's probably the behavior most users would expect IMO, and we don't need to add a dependency on gulp-if when using glob patterns.

sindresorhus commented 7 years ago

I think the best solution would be pushing directories to the stream only if there are some file changes in them

I think that makes the most sense too.

sindresorhus commented 7 years ago

@chris54721 Are you interested in updating your PR?

chris54721 commented 7 years ago

At the moment unfortunately I'm a bit busy IRL, I should be able to work on it in a couple of weeks.

sindresorhus commented 7 years ago

Alright. I'm closing this for now. But ping me here when you're ready and I'll reopen ;)

benjaminblack commented 7 years ago

Workaround using gulp-if:

[...].pipe(gulpIf(f => !f.isDirectory(), gulpChanged([...])))