Skip to content

chore(build): remove use of q.denodeify - #6546

Closed
pkozlowski-opensource wants to merge 1 commit into
angular:masterfrom
pkozlowski-opensource:remove_q_denodeify
Closed

pkozlowski-opensource wants to merge 1 commit into
angular:masterfrom
pkozlowski-opensource:remove_q_denodeify

Conversation

@pkozlowski-opensource

Copy link
Copy Markdown
Member

This change also makes webpack properly reject
promise on build errors

@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer and removed cla: yes labels Jan 18, 2016
@pkozlowski-opensource

Copy link
Copy Markdown
Member Author

This change was triggered by Igor's comment

@IgorMinar

Copy link
Copy Markdown
Contributor

Why not use https://www.npmjs.com/package/denodeify

@pkozlowski-opensource

Copy link
Copy Markdown
Member Author

Why not use https://www.npmjs.com/package/denodeify

Initially I didn't use denodeify since WebPack has a peculiar way of reporting errors (it doesn't set error arg in a callback, even if there is an error! see: pkozlowski-opensource@c61e207#diff-01f00c1c52bfe3be6b74cb139f885172R22)

But I can see now that denodeify has an option of passing a "filter" function:
https://github.com/matthew-andrews/denodeify#advanced-usage so we could use it, technically speaking. Having said this I'm not sure if it is worth the trouble of adding an additional dependency.

What is your preference?

@IgorMinar

Copy link
Copy Markdown
Contributor

Aren't there some other places where we use Q.denodeify? If so we should
replace them all with denodeify and drop Q.
On Tue, Jan 19, 2016 at 1:01 AM Pawel Kozlowski [email protected]
wrote:

Why not use https://www.npmjs.com/package/denodeify

Initially I didn't use denodeify since WebPack has a peculiar way of
reporting errors (it doesn't set error arg in a callback, even if there is
an error! see: pkozlowski-opensource/angular@c61e207
#diff-01f00c1c52bfe3be6b74cb139f885172R22
pkozlowski-opensource@c61e207#diff-01f00c1c52bfe3be6b74cb139f885172R22
)

But I can see now that denodeify has an option of passing a "filter"
function:
https://github.com/matthew-andrews/denodeify#advanced-usage so we could
use it, technically speaking. Having said this I'm not sure if it is worth
the trouble of adding an additional dependency.

What is your preference?


Reply to this email directly or view it on GitHub
#6546 (comment).

@pkozlowski-opensource

Copy link
Copy Markdown
Member Author

Aren't there some other places where we use Q.denodeify?

Nope, I could find any other uses of Q.denodeify in the angular repo.

@alxhub

alxhub commented Feb 1, 2016

Copy link
Copy Markdown
Member

@pkozlowski-opensource, can you rebase this? Thanks!

@vicb vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 8, 2016
@vicb

vicb commented Feb 8, 2016

Copy link
Copy Markdown
Contributor

@pkozlowski-opensource could you please rebase ?

@pkozlowski-opensource
pkozlowski-opensource force-pushed the remove_q_denodeify branch 2 times, most recently from 018b7c5 to 12975ce Compare February 24, 2016 09:37
This change also makes webpack properly reject
promise on build errors
@pkozlowski-opensource pkozlowski-opensource removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 24, 2016
@pkozlowski-opensource

Copy link
Copy Markdown
Member Author

@alxhub @vicb rebased & green. Sorry for the delay, it kind of slipped from my radar.

@mhevery mhevery closed this in b2db640 May 20, 2016
KiaraGrouwstra pushed a commit to KiaraGrouwstra/angular that referenced this pull request Jun 10, 2016
This change also makes webpack properly reject
promise on build errors

Closes angular#6546
@angular-automatic-lock-bot

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants