Maniphest T190141

Review promise usage and coalesce unneeded .then blocks
Closed, ResolvedPublic2 Estimated Story Points
Actions

Assigned To

Authored By

	• Jhernandez
	Mar 20 2018, 12:51 PM

Description

Seen in src/ui/renderer.js

	return wait( 200 )
		.then( function () {
			bindBehavior( preview, behavior );
		} )
		.then( function () {
			behavior.previewShow( token );
		} );

This two .then blocks are unnecessary and everything can be done in a single .then block as the statements in them are sync and are not returning any promises.

  	return wait( 200 )
  		.then( function () {
  			bindBehavior( preview, behavior );
- 		} )
- 		.then( function () {
  			behavior.previewShow( token );
  		} );

Inspect the Popups codebase for .then usages and coalesce misuses of promises into single .then blocks

AC

Codebase has been inspected
Repeated unnecessary .then blocks have been merged

Details

	Subject	Repo	Branch	Lines +/-
	Coalesce and cleanup use of `then` blocks	mediawiki/extensions/Popups	master	+8 -10

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Resolved		ovasileva	T192622 [EPIC] Page previews post-deploy cleanup
		Resolved		• Niedzielski	T190141 Review promise usage and coalesce unneeded .then blocks

Event Timeline

• Jhernandez created this task.Mar 20 2018, 12:51 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 20 2018, 12:51 PM

• Niedzielski added a project: Technical-Debt.Mar 20 2018, 1:15 PM

Jdlrobson moved this task from Incoming to Upcoming on the Web-Team-Backlog board.Mar 20 2018, 6:03 PM

Jdlrobson moved this task from Upcoming to Needs Prioritization on the Web-Team-Backlog board.

Anyway we can detect this programmatically via linting?

How can we grep to confirm we have changed all the patterns and are done?

In T190141#4069129, @Jdlrobson wrote:

Anyway we can detect this programmatically via linting?

I don't think so, this is just programming and understanding what Promises do. It is like if I choose to have 2 for loops or if statements instead of one to do one thing. Code review and education is the way to fix this.

How can we grep to confirm we have changed all the patterns and are done?

I'm not sure this is an automatable problem. I think we have to trust the inspecter to have checked the promise usage in the codebase, or the signoffer should check themselves at quick glance.

Jdlrobson moved this task from Needs Prioritization to Triaged but Future on the Web-Team-Backlog board.Mar 21 2018, 8:15 PM

Jdlrobson moved this task from Triaged but Future to Upcoming on the Web-Team-Backlog board.Mar 27 2018, 3:03 PM

Jdlrobson triaged this task as Medium priority.Mar 27 2018, 4:29 PM

we'll check in after standup about implementation details

ovasileva added a project: Readers-Web-Kanbanana-Board-Old.Apr 18 2018, 5:48 PM

ovasileva moved this task from Upcoming to 2017-18 Q4 on the Web-Team-Backlog board.Apr 19 2018, 5:01 PM

Jdlrobson added a parent task: T192622: [EPIC] Page previews post-deploy cleanup.Apr 20 2018, 12:50 AM

Jdlrobson moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.May 3 2018, 8:28 PM

Change 430809 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Coalesce and cleanup use of then blocks

https://gerrit.wikimedia.org/r/430809

gerritbot added a project: Patch-For-Review.May 3 2018, 9:18 PM

Jdlrobson moved this task from Doing to Needs Code Review on the Readers-Web-Kanbanana-Board-Old board.May 3 2018, 9:18 PM

Krinkle awarded a token.May 4 2018, 11:53 PM