Page MenuHomePhabricator

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

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

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 20 2018, 12:51 PM
Jdlrobson moved this task from Upcoming to Needs Prioritization on the Readers-Web-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?

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 triaged this task as Medium priority.Mar 27 2018, 4:29 PM
Jdlrobson set the point value for this task to 2.Mar 27 2018, 4:50 PM

we'll check in after standup about implementation details

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

Jan said he'd review this for me.

Change 430809 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Coalesce and cleanup use of then blocks

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

Jdlrobson reassigned this task from Jdrewniak to Niedzielski.Jun 4 2018, 5:09 PM
Jdlrobson added a subscriber: Jdrewniak.
Niedzielski closed this task as Resolved.Jun 4 2018, 5:41 PM

I grepped for "then" and reviewed all consecutive usages. Looks good