-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Dependencies] [Security] Upgrade to Gulp4 #6566
Conversation
This is great this will help a bunch! |
May be worth waiting until this is merged to fully resolve all audit issues on dependencies |
I wasnt able to get the tasks to run with this PR. Is this a work in progress? |
Alright gulp-copy 4 is merged I'll take another look now to make sure the tasks run |
@jlukic Okay this is now complete. You can run |
@acconrad Great, I will take a look at this when I have some time! |
@acconrad I just did some testing and it all looks good. The only problem I can find is when running |
Cool @hammy2899 fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@hammy2899 added a commit that should get your PR to work now :) |
All of the calc variables need to pull from their respective variables (instead of being hard-coded) otherwise they wont adjust properly with user themes. You'd want to be able to, for example change |
Maybe related to |
Good find @jlukic I had trouble finding that resolution, all fixed now! |
I've merged #6512 which updates calcs for compatibility with less 3.5. Can you take a look at that and see if it can fit into your PR? A simple way to confirm is to inspect an |
"gulp-debug": "*", | ||
"gulp-git": "*", | ||
"gulp-tap": "*", | ||
"merge-stream": "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was in both dependencies and dev dependencies and the latest version right now is 1.0.1 anyway so you shouldn't have this in both sections, that will trigger an NPM warning
@jlukic okay merged in. i verified everything looked right and the build still passes when building from npm and gulp. |
sorry about all of the bumps, this was during Github's outage this weekend and it kept telling me that no comments were going through (I even tried deleting all of them and they keep coming back), @jlukic @hammy2899 ready to merge when you are |
I was on vacation with family in Ireland, will probably have a chance to look this weekend or next. |
This ready to merge? |
all set @pgobin |
@acconrad Have you noticed From
For me, If it indeed does, a solution might look something like the following:
For the CSS watch and the remaining watches also. |
@pgobin it wasn't there before I started this PR and this PR doesn't touch |
Are we ready to merge this? Still would love to get this one in for my team! |
@jlukic? @levithomason? Can this be merged? Just this and a patch release would get the library building without security warnings... |
Pinging @jlukic and @hammy2899 ... this has been ready for 2 months and could have positive security impacts for lots of folks :) |
Upgrade to Gulp 4.
"gulp-clone": "^2.0.1", | ||
"gulp-concat": "^2.6.1", | ||
"gulp-concat-css": "^3.1.0", | ||
"gulp-copy": "4.0.1", | ||
"gulp-dedupe": "0.0.2", | ||
"gulp-flatten": "^0.4.0", | ||
"gulp-header": "^2.0.5", | ||
"gulp-help": "^1.6.1", | ||
"gulp-help": "github:chmontgomery/gulp-help#gulp4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just reviewing these changes to merge into my own fork and saw this. Given that this PR is intended to solve security warnings, I don't think we can trust adding a dependency on a floating github branch. It could be modified at any time, making builds unrepeatable and adding an attack vector.
Is there no published version that can be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is never going to be a gulp4 release of gulp-help
chmontgomery/gulp-help#31 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about referencing the commit hash of the HEAD of that branch?
This should work (never tried it though):
github:chmontgomery/gulp-help#194e80e0545ff6af5d10eeba7e224d0da71eb8d3
Not sure the github
dependency is needed either. This is standard:
git@github.com:github:chmontgomery/gulp-help.git#194e80e0545ff6af5d10eeba7e224d0da71eb8d3
E2A: yup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@badgerwithagun You could also check out the Fomantic-UI fork at https://github.com/fomantic/Fomantic-UI . We already have updated to gulp 4 and gulp-help will be removed with the next release
"wrench-sui": "^0.0.3", | ||
"yamljs": "^0.3.0" | ||
}, | ||
"devDependencies": { | ||
"github": "*", | ||
"github": "^14.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above. Not certain that this is needed.
Is there anything that stops this PR from being merged? Any help needed? |
@maxhq Give https://github.com/fomantic/Fomantic-UI a try it has gulp4 support and is activity maintained |
ping |
@acconrad This will most likely never be merged, its over 1 year now since last release of SUI (https://github.com/Semantic-Org/Semantic-UI/releases) I would recommend anyone looking at this to take a look at Fomantic-UI the official community fork which has gulp4 support. |
Upgraded yesterday to Fomantic, supersmooth, no issues |
This fixes multiple bugs (#6549 #5762 #6214 ) and implements #3793 by upgrading from Gulp
3.9.1
to4.0.0
which was released a few months ago.The primary goal of this is to incorporate a series of vulnerability fixes (
minimatch
,hoek
, andlodash
) that were introduced within the dependencies ofgulp
, and as a result were patched starting in Gulp v4.The basic upgrade flow works like this:
3.9.1
to4.0.0
gulp-help
to the version that specifically supports Gulp4 (1.6.1
tochmontgomery/gulp-help#gulp4
)gulp-concat-css
,gulp-copy
,gulp-less
,gulp-watch
, andgithub
)run-sequence
which is now deprecated and natively replaced withgulp.series
calc()
errors that arose out of interpolated strings with the latest version ofgulp-less
With those tasks complete,
npm install
works and completes as intended with no errors and 0 security vulnerabilities.