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

[epic] vinyl-fs version 3.0.0 #1604

Closed
15 tasks done
phated opened this issue Apr 15, 2016 · 53 comments
Closed
15 tasks done

[epic] vinyl-fs version 3.0.0 #1604

phated opened this issue Apr 15, 2016 · 53 comments

Comments

@phated
Copy link
Member

phated commented Apr 15, 2016

I'm creating this issue to get some more visibility on outstanding issues in vinyl-fs for it to reach 3.0 (which we'd like to ship with gulp 4).

If you are able to tackle any of these, the help would be very much appreciated.

@phated phated added this to the gulp 4 milestone Apr 15, 2016
@phated
Copy link
Member Author

phated commented Apr 25, 2016

Updated! We've had some excellent contributions by @hgwood and @tmcgee123
Some of the improvements brought to light a couple of other things we need help with.

@phated
Copy link
Member Author

phated commented Jun 28, 2016

Almost all these are wrapping up. We need to get some more tests in place for the changes and solidify the "Support functions for all options" stuff.

Still looking for someone to tackle the uid/gid stuff.

@odino
Copy link

odino commented Jul 12, 2016

What about a smaller update to vinyl? With this issue on minimatch everyone gets lots of sec. alerts :)

@phated
Copy link
Member Author

phated commented Jul 12, 2016

@odino no, those alerts don't matter. If you are passing user input directly to gulp, you have bigger problems.

@odino
Copy link

odino commented Jul 12, 2016

That's true, but those alerts will keep coming due to gulp requiring the
library. So even if this has never been exploited people are gonna
"wtf?!?!" anyway unless they really dig into it and figure out that this is
not an issue at all.

If there's an easy way to upgrade to a version that has the patch I would
suggest to go for it, else if its a big PITA then never mind :)
On Jul 12, 2016 1:40 PM, "Blaine Bublitz" notifications@github.com wrote:

@odino https://github.com/odino no, those alerts don't matter. If you
are passing user input directly to gulp, you have bigger problems.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1604 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAUC5BPlBGK1FkEyh0WVf2e_q6GkZku2ks5qU2ElgaJpZM4IIqCY
.

@strugee
Copy link

strugee commented Jul 14, 2016

@phated the UID/GID item should be checked off, no?

@ilanbiala
Copy link

@phated any update on ^^?

@atomantic
Copy link

atomantic commented Jul 21, 2016

re @odino and security fix. this could be fixed just by updating the version of vinyl-fs that gulp installs. Currently you get vinyl-fs@^0.3.0 which translates to a really old version of vinyl-fs (0.3.14).

This is a problem for anyone requiring nsp check to pass for a build to succeed.

@Den-dp
Copy link

Den-dp commented Jul 21, 2016

this could be fixed just by updating the version of vinyl-fs

@atomantic a lot of things was changed since vinyl-fs@0.3.14 https://github.com/gulpjs/vinyl-fs/blob/master/CHANGELOG.md#v0314-20150921-2357-0000 so it is impossible just bump the version.
In fact, migration to vinyl-fs@3.0.0 is a thing and the whole point of this issue.

@phated
Copy link
Member Author

phated commented Jul 21, 2016

@strugee @ilanbiala thanks, I've updated the list. However, a few more things had to be added to account for things I've found during the test refactor.

@atomantic
Copy link

got it. Yeah, I noticed tests pass as long as I upgrade vinyl-fs to latest version and include glob-watcher 0.0.8--but that's still a really old version of glob-watcher. glob-watcher@3.0.0 has some differences that break it.

@phated
Copy link
Member Author

phated commented Jul 21, 2016

@atomantic the gulp tests are just a smoke test because each underlying library has a full test suite that we aren't going to duplicate. Extreme amounts of stuff has changed and you should not assume everything will just work.

@phated
Copy link
Member Author

phated commented Jul 22, 2016

I've submitted the test refactor for review at gulpjs/vinyl-fs#194 - I'd like to get feedback before I push it so feel free to comment.

@pgilad
Copy link
Contributor

pgilad commented Aug 31, 2016

Hey - any news on this? gulp is bringing in an outdated version of minimatch (with security issues)..

@phated
Copy link
Member Author

phated commented Aug 31, 2016

@pgilad I'm pretty diligent about updating this thread when there are updates, so no. You can follow the linked issues to see progress on the outstanding items (and even contribute). The minimatch warning you mention doesn't mean anything because you aren't passing raw user input to the gulp methods (or you shouldn't be).

@addyosmani
Copy link

@phated First, thanks for all the awesome contributions you continue to make to Gulp. Second, looking at the prepareWrite note it appears this is done as of gulpjs/vinyl-fs@cc99707 and the associated tests also landed. Was there more work to do before it could be checked off the list?

@phated
Copy link
Member Author

phated commented Sep 15, 2016

@addyosmani unfortunately that commit didn't solve everything and caused problems with the symlink method so it was never merged. There are some more option normalizations that are in progress, so I have been focusing on the glob-stream and vinyl dependencies.

@phated
Copy link
Member Author

phated commented Feb 25, 2017

Thanks but we have many issues opened throughout the organization.

You can see everything labeled as "help wanted" at https://github.com/issues?utf8=%E2%9C%93&q=is%3Aopen+org%3Agulpjs+label%3A%22help+wanted%22+

We are tackling some of them this weekend at HackIllinois

@CKGrafico
Copy link

Amazing work, I'll wait to next week. Good luck at HackIllinois

@phated
Copy link
Member Author

phated commented Mar 31, 2017

This is getting really close. I need help with gulpjs/vinyl-sourcemap#19

@anru
Copy link

anru commented Jun 28, 2017

Any progress on this ? Looks like almost things from list are already done. May be I can help somehow ?

gulp still have very old vinyl-fs dependency, and I get following warning each time I install/upgrade gulp:
npm WARN deprecated graceful-fs@2.0.3: graceful-fs version 3 and before will fail on newer node releases. Please update to graceful-fs@^4.0.0 as soon as possible

├─┬ gulp@3.9.1
│ └─┬ vinyl-fs@0.3.14
│ ├─┬ glob-watcher@0.0.6
│ │ └─┬ gaze@0.5.2
│ │ └─┬ globule@0.1.0
│ │ └─┬ glob@3.1.21
│ │ └── graceful-fs@1.2.3

@avegancafe
Copy link

@anru If you check out that link phated posted there's a bunch of issues left in the org, I think those are all related to the upgrade

@mcdado
Copy link

mcdado commented Jun 28, 2017

@anru I believe the roadmap is here: #1945

More than the warning the main issue of the current version is the incompatibility with Node > 6. Later this year 8 will be LTS and then it will become more problematic to have gulp as a dependency.

I previously said I'm willing to help, I'll probably take a look at doc related issues.

@anru
Copy link

anru commented Jun 28, 2017

#1945 is gulp 4 roadmap. And may be we could release gulp 3 with updated vinyl-fs dependency. Or is it will be more simple to help with gulp 4 where vinul-fs is already updated. What will be more quick/easy ?
I'm asking because there are no incomplete issues in issue description, except Fix Symlink TODOs (issues need to be created)

@strugee
Copy link

strugee commented Jun 28, 2017

@anru @mcdado see #1571. The gulp team is aware of the graceful-fs problems, but it's impossible to upgrade Vinyl without breaking changes. Hence, gulp 4.

@subesokun
Copy link

subesokun commented Oct 4, 2017

Any updates on this? The NSP check fails on all my projects which are using Gulp when scanning also dev dependencies :( https://nodesecurity.io/advisories/118

@ghost
Copy link

ghost commented Oct 4, 2017

@subesokun
It seems like that vulnerability requires an attacker to be able to pass an arbitrary glob string to minimatch... which seems like a bad idea in general.

@avegancafe
Copy link

Is the "Fix Symlink TODOs (issues need to be created)" item the only thing that's left to do here?

@phated
Copy link
Member Author

phated commented Oct 6, 2017

@kyleholzinger yes, it is a tough problem we are trying to solve over on the vinyl-fs repo.

@nwhitmont
Copy link

What can I do to help move this forward?

@phated
Copy link
Member Author

phated commented Oct 23, 2017

@nwhitmont the work is happening at gulpjs/vinyl-fs#254

@phated
Copy link
Member Author

phated commented Nov 10, 2017

We've wrapped up the symlink/junction work. There's just some documentation/rebasing/cleanup work to do on vinyl-fs before the 3.0 release.

If you are interested in this stuff, please go try out the master branch and give us feedback - there are some big changes over there.

@phated
Copy link
Member Author

phated commented Nov 30, 2017

Documentation has been finalized. Rebase has been completed but it needs review. Check out gulpjs/vinyl-fs#284 if you are interested in reviewing.

@phated
Copy link
Member Author

phated commented Dec 5, 2017

Based on the reactions, I expected more review/feedback on the rebase ☹️

Anyway, 3.0.0 has been published. Changelog at https://github.com/gulpjs/vinyl-fs/releases/tag/v3.0.0

@phated phated closed this as completed Dec 5, 2017
@CKGrafico
Copy link

Thanks for your awesome job @phated

@mcdado
Copy link

mcdado commented Dec 6, 2017

I tried to have a look but it was a bit overwhelming not knowing anything of the internals.

I second @CKGrafico, thank you and the whole team!

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

No branches or pull requests