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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dependencies and move to ESM #267

Merged
merged 12 commits into from Dec 12, 2022
Merged

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
@@ -83,18 +83,18 @@ The hash of each rev'd file is stored at `file.revHash`. You can use this for cu
### Asset manifest

```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
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.

None yet

2 participants