Maniphest T206027

Collapse mobile.search.util into mobile.startup
Closed, ResolvedPublic3 Estimated Story Points
Actions

Description

mobile.search.util is defined as:

"mobile.search.util": {
	"targets": [
		"mobile",
		"desktop"
	],
	"dependencies": [
		"mobile.startup"
	],
	"scripts": [
		"resources/mobile.search.util/extendSearchParams.js"
	]
}

That is, it consists of one JavaScript file and depends on mobile.startup.

It is referenced only by MobileFrontend, mobile.watchstar, mobile.search.api, and mobile.nearby. The first two already each depend on mobile.startup and mobile.nearby depends on mobile.init which depends on mobile.startup. mobile.watchlist has an implicit dependency on mobile.search.util but already depends on mobile.startup.

This task encompasses the following work:

Move extendSearchParams.js and extendSearchParams.test.js to mobile.startup.
Update all M.define() / require() calls from 'mobile.search.util/extendSearchParams' to 'mobile.startup/extendSearchParams'. mobile.startup/extendSearchParams.js' M.define() call is moved to mobile.startup.js as per usual.
Update mobile.startup/extendSearchParams.js to use module.exports, remove the top-level IIFE and replace parameters with file scope variables, and replace other M.require() calls with Node.js require().
Add a Node.js require('./extendSearchParams.js') to the end of the growing module list in mobile.startup.js.
Delete the extension.json entry for mobile.search.util.
Easy, peasy.

Acceptance criteria

Update any dependencies to mobile.search.util to point to mobile.startup
Remove the mobile.search.util module (there is no impact on caching here as mobile.search.util is never added directly to the HTML via an addModules call)
Follow the deprecation policy to ensure compatibility between old and new modules. The new module should be renamed and defined via M.define like so:

M.define( "mobile.startup/extendSearchParams" ).deprecate( "mobile.search.util/extendSearchParams" )

Update existing users to use the new module name.

Sign off step

Create task to cleanup M.deprecate calls post-webpack migration (see T208915)
Progress is updated.

Details

	Subject	Repo	Branch	Lines +/-
	Collapse mobile.search.util into mobile.startup	mediawiki/extensions/MobileFrontend	master	+196 -190
	Access extendSearchParams through appropriate (newer) module	mediawiki/extensions/MobileFrontend	master	+7 -4

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	Jdlrobson	T195473 [GOAL] Invest in the MobileFrontend & MinervaNeue frontend architecture
Resolved	Jdlrobson	T195475 [EPIC] Automate asset bundling in MobileFrontend
Resolved	Jdlrobson	T207787 [EPIC] Reduce the amount of modules in MobileFrontend
Resolved	Jdlrobson	T206027 Collapse mobile.search.util into mobile.startup

Event Timeline

• Niedzielski created this task.Oct 2 2018, 8:29 PM

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptOct 2 2018, 8:29 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

@ovasileva, @Jdlrobson, please consider this task for todo!

Jdlrobson moved this task from Incoming to Triaged but Future on the Web-Team-Backlog board.Oct 2 2018, 8:34 PM

Yup! This one's on my roadmap as soon as we finish the existing stuff!
https://docs.google.com/document/d/163tkS5i5dUNhMbHV9GNu72TPohDddEO5P2LkICySxIM/edit#

I think it may also be possible to increase the scope of this task.

Awesome. Any reason to increase scope? I know that task wrangling is work in itself but this kind of feels like a 2 or 3-pointer to me as its currently laid out given complexity, risk, and the staggered patches proposed. If we keep the task distinct, another dev could work in parallel on a separate module collapse as outlined in your plan.

Maybe not. What would help is understanding what this entails and the risks relating to caching and usage of the module via M.define (which we'd need to use deprecate for). Let's work all this out upfront so this change is mechanical and we know all about the risk.

Reviewed and updated acceptance criteria. Let's keep this simple and see what we learn from the exercise. Hopefully this will be straightforward.

Jdlrobson mentioned this in T205128: Bundle Hogan.js.Oct 3 2018, 8:46 PM

Copy extendSearchParams.js and move extendSearchParams.test.js to mobile.startup.

@Jdlrobson, do we need a patch to delete the original and its extension.json entry?

In T206027#4640102, @Niedzielski wrote:

Copy extendSearchParams.js and move extendSearchParams.test.js to mobile.startup.

@Jdlrobson, do we need a patch to delete the original and its extension.json entry?

Nope this can be done right away. Tthere is no impact on caching here as mobile.search.util is never added directly to the HTML via an addModules call)

ag mobile.search.util

shows that this is never referenced in PHP.

I'm trying to understand the deprecation policy. Why do we need M.define( "mobile.startup/extendSearchParams" ).deprecate( "mobile.search.util/extendSearchParams" )? If we remove the underlying code, it's not deprecated, it's gone!

Two reasons

Some gadget may be making use of it. Gives them a heads up it's about to break
More important: ResourceLoader caches modules in localStorage.

Let's say an old version of resources/mobile.search.api/SearchGateway.js is loaded from localStorage it will make a call to

extendSearchParams = M.require( 'mobile.search.util/extendSearchParams' );

Provided mobile.search.util is also in cache, no problem. However if mobile.search.util has been cleaned and a new mobile.startup module loaded, there is a risk of an exception. If the deprecate call has been used this will continue to work.

Not applicable here, but it will be for other modules - I'd rather not have to audit all extensions and skins for usages of MobileFrontend's modules every time we change something. Getting in the habit of using deprecate, we can do that audit and cleanup later.

@Jdlrobson, thanks! I think I missed the key point that .deprecate( "mobile.search.util/extendSearchParams" ) "reroutes". \o/

• Niedzielski updated the task description. (Show Details)Oct 3 2018, 9:33 PM

ovasileva set the point value for this task to 3.Oct 10 2018, 4:30 PM

Jdrewniak edited projects, added Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2); removed Web-Team-Backlog.Oct 16 2018, 1:26 PM

Jdlrobson moved this task from To Do to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 22 2018, 4:58 PM

Change 469119 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Collapse mobile.search.util into mobile.startup

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

Change 469120 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Access extendSearchParams through appropriate (newer) module

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

Jdlrobson moved this task from Doing to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 22 2018, 8:57 PM

Change 469120 abandoned by Jdlrobson:
Access extendSearchParams through appropriate (newer) module

Reason:
resource-modules does not allow this. squashed into parent patch

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

Jdlrobson added a parent task: T207787: [EPIC] Reduce the amount of modules in MobileFrontend.Oct 23 2018, 7:57 PM

• nray claimed this task.Oct 31 2018, 5:30 PM

• nray removed • nray as the assignee of this task.Oct 31 2018, 11:00 PM

• nray moved this task from Needs Code Review to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.

• nray subscribed.

Jdlrobson claimed this task.Nov 1 2018, 12:03 AM

Jdlrobson moved this task from Needs More Work to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.