Support Gulp 3.9.1 and 4.0.0, Node.js 10 #64
+171
−108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
May 29, 2018 13:31
Eliminates a lot of common boilerplate from each test case, and fixes other issues. Also adds assertions to ensure events are triggered as expected. Previously any assertion errors would technically go uncaught. The more proper way to report them in async tests is to set up a try/catch and pass the error to `done()`. Both stdout and stderr are now captured, and the "should fail when running a generator without slushfile" now fails. Previously, when slush failed in this test, stdout was empty while stderr was ignored due to a failure in liftoff (`cli.launch()`). The `data` variable used to capture stdout was empty, which caused `should.match` to always pass. All output is now written to stderr when a test fails to facilitate stack trace debugging (as with the `cli.launch()` failure).
Gulp 4.0.0 switched its task execution engine from `orchestrator` to `undertaker`. As a result, certain methods and events from Gulp 3.9.1 upon which `slush` depended disappeared: gulpjs/gulp#755 Supporting Gulp 4.0.0 is important because Node 10 broke the `graceful-fs` package upon which Gulp 3.9.1 depends. While there's a workaround (updating the `natives` package), it places a burden on users that still doesn't guarantee that Gulp 3.9.1 will remain future-proof: gulpjs/gulp#2146 (comment) gulpjs/gulp#2162 (comment) nodejs/node#19786 (comment) As it turned out, the changes required to support both versions were fairly straighforward, and should ensure that Slush remains future-proof until the next major Gulp update. NOTE: The test tasks are now all asynchronous via a `done` callback, since Gulp 4 doesn't support synchronous tasks. Any synchronous slushfile task will need to be updated.
liftoff will take some work, as the API has changed between 0.10.0 and 2.5.0.
Fixes the "should fail when running a generator without slushfile" test
case.
As noted in the commit that updated runSlush() from test/slush.js and
caused the failure, slush was actually failing without the expected "No
slushfile found" message. It was failing with a stack trace that
started with:
path.js:39
throw new ERR_INVALID_ARG_TYPE('path', 'string', path);
^
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type
string. Received type object
at assertPath (path.js:39:11)
at Object.extname (path.js:1375:5)
at Liftoff.buildEnvironment (/Users/mbland/src/mbland/slush/node_modules/liftoff/index.js:111:50)
at Liftoff.launch (/Users/mbland/src/mbland/slush/node_modules/liftoff/index.js:154:22)
at Object.<anonymous> (/Users/mbland/src/mbland/slush/bin/slush.js:57:5)
This points to a bug in the previous version of liftoff whereby the the
search for `slushfile.js` found nothing, resulting in a `configPath` of
`null`. liftoff passed this `null` to `Object.extname`, resulting in
this stack trace.
liftoff v2.5.0 doesn't have this problem, but it does have a slightly
different API. With the changes to `bin/slush.js`, this test case now
passes along with the rest of the suite.
The previous versions, 0.10 and 0.11, are years out of date. Using the latest versions per the Travis docs should fix the build: https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#Specifying-Node.js-versions
added 2 commits
May 30, 2018 08:38
No behaviorial changes.
|
Travis was failing before because it was configured to use Node 0.10 and 0.11. I've pushed a commit to update the build to use the latest stable and LTS versions instead, and it now passes. (Well, it passes now that Travis's problem yesterday with hitting npm rate limits has passed, and I pushed a couple more touch-up commits.) |
|
@mbland hi could you fix the conflicts and verify that Slush works with the latest Gulp release? Sidenote: I'm really slow 😞 , because of reasons, at responding to tickets and looking into my open source repos (especially those I don't use anymore myself). I would gladly add more maintainers to my repos to help keep them updated! Please let me know if someone is interested in this. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I started this PR before I noticed #31, but derived some inspiration from it. However, rather than rely entirely on gulp-cli, this PR keeps most of the existing logic intact, as I thought maybe it's better to preserve
slushin the process table and to preserve the existing output format. (gulp-cli doesn't have hooks to change the parameters it passes to liftoff, and its events are slightly different.)Also, I've added extra assertions to the test cases and updated the test tasks to be asynchronous, as Gulp 4 doesn't support synchronous tasks. (I hope most slushfile tasks are async already.)
I'm also aware of #62 and am happy for that to go in before or after.
This comment from d92d227 gives the rest of the context:
Gulp 4.0.0 switched its task execution engine from
orchestratortoundertaker. As a result, certain methods and events from Gulp 3.9.1 upon whichslushdepended disappeared:Supporting Gulp 4.0.0 is important because Node 10 broke the
graceful-fspackage upon which Gulp 3.9.1 depends. While there's a workaround (updating thenativespackage), it places a burden on users that still doesn't guarantee that Gulp 3.9.1 will remain future-proof:As it turned out, the changes required to support both versions were fairly straightforward, and should ensure that Slush remains future-proof until the next major Gulp update.
NOTE: The test tasks are now all asynchronous via a
donecallback, since Gulp 4 doesn't support synchronous tasks. Any synchronous slushfile task will need to be updated.