Add registerAnchorMutator to navigation service #17522

alabiaga · 2018-08-15T19:54:58Z

add a means of registering a location mutator. An achor mutator modifies the anchor object on the onclick event, executing based on it's priority. The number 1 being of highest priority.

e.g.
given location href is http://www.ampproject.org

navigation.registerAnchorMutator((location) => { location.href += '&first=1'; return location; }, 1); navigation.registerAnchorMutator((location) => { location.href += '&second=2'; return location; }, 2);

will result in a location href inline update evaluating to, http://www.ampproject.org&first=1&second=2

Address #16719

alabiaga · 2018-08-15T19:59:06Z

cc\ @calebcordry

Caleb, let me know if API makes sense and fits your needs.

calebcordry · 2018-08-15T20:22:39Z

src/service/navigation.js

+   */
+  registerLinkRule(callback, priority) {
+    user().assert(!this.isLinkRulePriorityUsed_(priority),
+        'Rule with same priority is already in use.');


I think maybe we should allow more than one rule with the same priority. If not, every time a new use case is introduced we will have to add another entry to the enum.

right. currently..any number is open for anyone to use. I initially thought of reserving numbers for each extension or use case but as you mentioned, I see no issue with allowing some rules to have the same priority or maybe have a concept of no priority and it can be executed in any order..left for last. As discussed from the meeting we need to prioritize the linker URL transformations vs the skimlinks URL transformations. If so then perhaps, reserve a number now for the linker link rule priority? and make priority optional.

I think we need priority uniqueness. Otherwise, isn't the order of invocation for handlers with the same priority indeterminate?

right..I think we should have priority uniqueness but was concerned with users who want to register an anchor mutation and didn't care for priority, for example if simply adding a queryParam with no concern of where it's placed. But we can start with reserving a priority for users of this and change it if users in the future show similar concerns.

@calebcordry When using the API, just define a number and I can follow with a separate PR with the priority enum, eslint rules and/or proper js annotations to ensure that users define and reserve a priority number in an enum.

Agree, I was thinking that people may not care specific order just that one group executes before the other. I guess we can cross that bridge when when see more than two use cases 🙂.

@alabiaga Sounds good.

calebcordry · 2018-08-15T20:23:42Z

src/service/navigation.js

+   * @return {!Location}
+   */
+  transform(location) {
+    return this.callback_(location);


We talked about using element here. Location seems fine to me, but is there any downside you can ~~thing~~ think of @lannka @choumx ?

Right, I don't remember the use case for why we needed to use the element here and thought that narrowing it to the scope of the location object would be ok. Lets see what @lannka @choumx say. Thanks for the feedback.

I think element is more flexible so that we can

read attributes for extra configurations later we might have

keep the original URL as an attribute of the element, and can be accessed by any following mutators

sounds good. made the change.

calebcordry · 2018-08-15T20:35:26Z

Thanks for getting this prioritized! 👍

lannka · 2018-08-15T22:02:30Z

extensions/amp-video/0.1/amp-video.js

@@ -103,6 +103,16 @@ class AmpVideo extends AMP.BaseElement {
    }
  }

+  /**


are these change from a bad merge?

yes, this along with base-element.js

lannka · 2018-08-15T22:05:08Z

src/service/navigation.js

+   * @return {!Location}
+   */
+  transform(location) {
+    return this.callback_(location);


I think element is more flexible so that we can

read attributes for extra configurations later we might have

keep the original URL as an attribute of the element, and can be accessed by any following mutators

lannka · 2018-08-16T20:54:53Z

src/service/navigation.js

+   * @return {!Element}
+   */
+  transform(el) {
+    return this.callback_(el);


this method might not need to return anything. unless you want to create a new Element instance, which I think is unnecessary and error-prone

yea you're right. removed.

lannka · 2018-08-16T23:21:40Z

src/service/navigation.js

+  registerAnchorMutator(callback, priority) {
+    user().assert(!this.isAnchorMutatorPriorityUsed_(priority),
+        'Mutator with same priority is already in use.');
+    this.registeredAnchorMutators_.push(new AnchorMutator(callback, priority));


instead of "check then push", you can do one scan, and splice in.
that will also save you from later sorting

dreamofabear · 2018-08-17T17:11:46Z

src/service/navigation.js

@@ -124,6 +124,9 @@ export class Navigation {
     * @private {?Array<string>}
     */
    this.a2aFeatures_ = null;
+
+    /** @const @private {!Array<!AnchorMutator>} */
+    this.registeredAnchorMutators_ = [];


Nit: this.anchorMutators_

dreamofabear · 2018-08-17T17:12:17Z

src/service/navigation.js

+    const am = new AnchorMutator(callback, priority);
+    if (this.registeredAnchorMutators_.length) {
+      this.registeredAnchorMutators_.splice(
+          this.findIndexToInsert(this.registeredAnchorMutators_, am), 0, am);


A simpler option is to use a sparse array and simply do:

user().assert(!this.anchorMutators[priority]); this.anchorMutators[priority] = callback;

Then just add a null check when you iterate over this.anchorMutators_.

This is definitely simpler and as discussed offline, I am not familiar with the performance of sparse arrays. Lets do this solution and follow up with research on what would be more performant. it also looks like forEach skips the empty cells so null check is not necessary.

dreamofabear · 2018-08-17T17:13:20Z

src/service/navigation.js

@@ -124,6 +124,9 @@ export class Navigation {
     * @private {?Array<string>}
     */
    this.a2aFeatures_ = null;
+
+    /** @const @private {!Array<!AnchorMutator>} */


ClickListener?

From your comment on using sparse arrays and the code sample..I take it that you wanted !Array and just do away with the AnchorMutator object.

alabiaga · 2018-08-20T16:25:33Z

Submitting for Caleb to use. Thanks all. Will fix any that will arise from his usage.

jridgewell · 2018-08-20T18:45:54Z

src/service/navigation.js

+    const transformedTarget = target;
+    this.anchorMutators_.forEach(callback => {
+      callback(target);
+      location = this.parseUrl_(target.href);


Why do we need to parseUrl every time?

jridgewell · 2018-08-20T18:46:20Z

src/service/navigation.js

@@ -253,8 +256,15 @@ export class Navigation {
      return;
    }

+    // Handle anchor transformations.
+    const transformedTarget = target;


This variable seems unnecessary?

jridgewell · 2018-08-20T18:47:42Z

src/service/navigation.js

+  registerAnchorMutator(callback, priority) {
+    user().assert(!this.anchorMutators_[priority],
+        'Mutator with same priority is already in use.');
+    this.anchorMutators_[priority] = callback;


This will make sparse arrays, which will cause forEach to be very slow. Use a binary-search to find the proper location, and use Array.p.splice to insert it instead.

#17522 (comment) 😛

I think this is fine since N is tiny, but if you really want you could use our existing PriorityQueue class that I wrote.

N doesn't matter here, only priority. Given that we can't register the same priority class twice, I expect we'll see odd numbers like 100, 200, meaning we'll be iterating 200 items to only find 2.

I meant N == array.length.

I expect we'll see odd numbers like 100, 200, meaning we'll be iterating 200 items to only find 2.

We can just as easily not do that by comment and example.

* fix test * fixes * change name as per choumx comment offline * fix * revert * name change * return element instead of location object * revert url.js changes * address lannka@ comments * fix check types * address comments * increase bundle size * address choumx comments * fix presubmit error

googlebot added the cla: yes label Aug 15, 2018

calebcordry reviewed Aug 15, 2018

View reviewed changes

alabiaga requested review from dreamofabear, lannka and jridgewell August 15, 2018 20:47

alabiaga changed the title ~~[WIP] Add registerLinkRule to navigation service~~ [WIP] Add registerLocationMutator to navigation service Aug 15, 2018

lannka reviewed Aug 15, 2018

View reviewed changes

alabiaga added 8 commits August 16, 2018 10:15

fix test

83463fb

fixes

23a959b

change name as per choumx comment offline

938a464

fix

ad43319

revert

8a0e99c

name change

f03301f

return element instead of location object

f960311

revert url.js changes

e1bc3f2

alabiaga changed the title ~~[WIP] Add registerLocationMutator to navigation service~~ Add registerLocationMutator to navigation service Aug 16, 2018

alabiaga changed the title ~~Add registerLocationMutator to navigation service~~ Add registerAnchorMutator to navigation service Aug 16, 2018

lannka reviewed Aug 16, 2018

View reviewed changes

alabiaga added 2 commits August 16, 2018 17:19

address lannka@ comments

a41e68b

fix check types

9c03746

lannka reviewed Aug 16, 2018

View reviewed changes

alabiaga added 2 commits August 17, 2018 13:05

address comments

3669344

increase bundle size

5c97652

dreamofabear reviewed Aug 17, 2018

View reviewed changes

alabiaga added 2 commits August 17, 2018 15:29

address choumx comments

2ef475d

fix presubmit error

59c13f5

calebcordry approved these changes Aug 20, 2018

View reviewed changes

alabiaga merged commit 12cb19c into ampproject:master Aug 20, 2018

jridgewell reviewed Aug 20, 2018

View reviewed changes

This was referenced Aug 20, 2018

Set limits to the allowed number set for priority in navigation.registerAnchorMutator #17612

Merged

Allow same priority when registering anchor mutators. #17634

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add registerAnchorMutator to navigation service #17522

Add registerAnchorMutator to navigation service #17522

alabiaga commented Aug 15, 2018 •

edited

alabiaga commented Aug 15, 2018

calebcordry Aug 15, 2018

alabiaga Aug 15, 2018

dreamofabear Aug 15, 2018

alabiaga Aug 16, 2018

calebcordry Aug 16, 2018

calebcordry Aug 15, 2018 •

edited

alabiaga Aug 15, 2018

lannka Aug 15, 2018

alabiaga Aug 16, 2018

calebcordry commented Aug 15, 2018

lannka Aug 15, 2018

alabiaga Aug 16, 2018

lannka Aug 15, 2018

lannka Aug 16, 2018

alabiaga Aug 16, 2018

lannka Aug 16, 2018 •

edited

dreamofabear Aug 17, 2018

alabiaga Aug 17, 2018

dreamofabear Aug 17, 2018

alabiaga Aug 17, 2018

dreamofabear Aug 17, 2018

alabiaga Aug 17, 2018

alabiaga commented Aug 20, 2018

jridgewell Aug 20, 2018

jridgewell Aug 20, 2018

jridgewell Aug 20, 2018

dreamofabear Aug 20, 2018

jridgewell Aug 20, 2018

dreamofabear Aug 20, 2018

@@ @@ -103,6 +103,16 @@ class AmpVideo extends AMP.BaseElement { @@
                   }
                 }
+                /**

Add registerAnchorMutator to navigation service #17522

Add registerAnchorMutator to navigation service #17522

Conversation

alabiaga commented Aug 15, 2018 • edited

alabiaga commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcordry Aug 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcordry commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka Aug 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented Aug 15, 2018 •

edited

calebcordry Aug 15, 2018 •

edited

lannka Aug 16, 2018 •

edited