Page MenuHomePhabricator

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

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.

Event Timeline

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!

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.

Jdlrobson updated the task description. (Show Details)Oct 3 2018, 8:45 PM
Jdlrobson moved this task from Needs Analysis to Upcoming on the Readers-Web-Backlog board.

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

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?

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

  1. Some gadget may be making use of it. Gives them a heads up it's about to break
  2. 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.

  1. 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

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

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

nray claimed this task.Oct 31 2018, 5:30 PM
nray removed nray as the assignee of this task.
nray added a subscriber: nray.

Change 469119 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Collapse mobile.search.util into mobile.startup

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson closed this task as Resolved.Nov 7 2018, 12:38 AM
Jdlrobson updated the task description. (Show Details)