Skip to content

Conversation

@pioug
Copy link
Contributor

@pioug pioug commented Dec 8, 2022

Thanks for releasing a new version of vinyl-file 🙏 This time, I want to update gulp-rev!

What's new?

  • Update all dependencies
  • Migrate to ESM
  • Migrate async ava tests using p-event
  • Fix linting errors with new xo

The tests are passing in my repo.

I also updated some examples in the readme but I am not too sure about the ones that mix ESM and CJS modules.

@sindresorhus
Copy link
Owner

Thanks. Would you be willing to move to use easy-transform-stream too? See: sindresorhus/gulp-strip-debug@a8d44b6

@pioug
Copy link
Contributor Author

pioug commented Dec 9, 2022

@sindresorhus Thanks for the suggestion! I didn't know about this package. I'll update the PR again over the weekend.

@pioug
Copy link
Contributor Author

pioug commented Dec 9, 2022

I migrated to easy-transform-stream but I have a problem with the flush function on Node 14 (tests are passing with Node 15+) 🤔

I think it works as expected once nodejs/node#34314 has landed on Node 15.

readme.md Outdated
```js
const gulp = require('gulp');
const rev = require('gulp-rev');
import {src, dest} from 'gulp';
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to keep gulp a default import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

➡️ 39e09fa

readme.md Outdated
exports.default = () => (
gulp.src('src/*.js')
exports.default = async () => {
const {default: rev} = await import('gulp-rev');
Copy link
Owner

Choose a reason for hiding this comment

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

Why dynamic import?

Copy link
Contributor Author

@pioug pioug Dec 11, 2022

Choose a reason for hiding this comment

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

➡️ 1d6480d

My mistake, I thought that we couldn't import the other packages of the examples using ES import

if (path.extname(file.path) === '.map') {
t.is(file.path, 'pastissada-d41d8cd98f.css.map');
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Do an assert for the correct file count too. Currently, if the stream never emits, it would not be caught and test would succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

➡️ 553466b

@sindresorhus
Copy link
Owner

I migrated to easy-transform-stream but I have a problem with the flush function on Node 14 (tests are passing with Node 15+) 🤔

I think we can just target Node.js 16. This is a dev tool anyway, not a reusable package.

@sindresorhus sindresorhus merged commit 1a9b1e3 into sindresorhus:main Dec 12, 2022
@sindresorhus
Copy link
Owner

Nice work 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants