Major code cleanup #13
Conversation
index.js
Outdated
| var os = require('os'); | ||
| var path = require('path'); | ||
|
|
||
| var Q = require('q'); |
There was a problem hiding this comment.
I think the Q.fcall(function () { }) could be replaced with new Promise(function (resolve, reject) { }) pretty easily to remove the dependency on Q in favour of native promises
There was a problem hiding this comment.
Yes, I actually wanted to do that. But then I noticed that all the tests of this module depend on Q-specific methods (e.g. fail instead of catch). Furthermore this module depends on fetch returning a Q instance too (again, calling fail on the result). So I thought I'd better play it safe in case the rest of the Cordova tooling relies on cordova-create returning Q instances.
But I'm definitely in favor of getting rid of Q. As a first step, I could remove all usage of Q-specific methods in this module, while still returning a Q instance to be safe until further investigations have been done.
To play it safe, cordova-create still returns a Q instance, so as to not break any callers that are relying on this implementation detail.
raphinesse
force-pushed
the
cleanup
branch
3 times, most recently
from
e881bb4
to
7795b64
Compare
There was so much wrong with the previous test: - I could not get it to fail, even w/out appending '@latest' in create - It relies on an active network connection and can take over 20s easily - It relies on knowing where fetch will save the downloaded template - It seems to test some kind of cache busting that should probably handled by fetch instead of create anyway The new test inherits only the last downside.
|
🎉 |
Major Cleanup reporting for duty! 🎖️
This PR does a lot of overdue house keeping. Removing dead code, factoring out repeated patterns, fixing some comments and other cleanup tasks.
I intentionally kept the commits separate for now because I think this rather big change set is more reviewable this way. If it is preferred, the commits can still be squashed in the end.
Fix error shadowing on failed fetch fixes an error that was thrown while handling another error.
None of the other commits should introduce any change in behavior.
The test duration was brought down to about half a second (was more than a minute sometimes) by mocking out any calls to
fetch.I'm looking forward to your feedback. I hope you are with me on this PR. I made a few breaking changes locally that I would like to build onto this.
Code coverage