Make share-tracking extension to get incoming/outcoming fragments #4019
Conversation
muxin
force-pushed
the
share-tracking-service
branch
2 times, most recently
from
36d1076
to
7b00ae3
Compare
muxin
force-pushed
the
share-tracking-service
branch
4 times, most recently
from
73b169c
to
83cd52a
Compare
muxin
changed the title
muxin
force-pushed
the
share-tracking-service
branch
from
83cd52a
to
0a6e561
Compare
|
@cramforce PTAL |
| @@ -115,6 +115,13 @@ app.use('/form/echo-json/post', function(req, res) { | |||
| }); | |||
| }); | |||
|
|
|||
| app.use('/share-tracking/get-outgoing-fragment', function(req, res) { | |||
| res.setHeader('Content-Type', 'application/json'); | |||
muxin
force-pushed
the
share-tracking-service
branch
from
0a6e561
to
d56fa43
Compare
| * outgoingFragment: string | ||
| * }} | ||
| */ | ||
| let ShareTrackingPostResponseDef; |
muxin
force-pushed
the
share-tracking-service
branch
2 times, most recently
from
73800ae
to
e63e593
Compare
| } | ||
|
|
||
| /** @override */ | ||
| createdCallback() { |
There was a problem hiding this comment.
Is there a reason to put this in #createdCallback instead of #buildCallback?
There was a problem hiding this comment.
I'm not sure about this. I saw AmpUserNotification did this.
@dvoytenko WDYT
There was a problem hiding this comment.
I'll move everything in buildCallback
muxin
force-pushed
the
share-tracking-service
branch
2 times, most recently
from
40ebfbd
to
5d91985
Compare
muxin
changed the title
|
Adressed all the comments in latest commit. PTAL |
| } | ||
|
|
||
| /** @override */ | ||
| isLayoutSupported(layout) { |
There was a problem hiding this comment.
Would you mind copying this to amp-experiment.js as well?
There was a problem hiding this comment.
I created a separate PR since this has nothing to do with share-tracking.
|
LGTM with one request |
|
|
||
| /** @private @const {string} */ | ||
| const TAG = 'amp-share-tracking'; | ||
|
|
||
| class AmpShareTracking extends AMP.BaseElement { | ||
| export class AmpShareTracking extends AMP.BaseElement { |
There was a problem hiding this comment.
Please mark this as @visibleForTesting
muxin
force-pushed
the
share-tracking-service
branch
from
d9b9c20
to
d6df2e6
Compare
|
PTAL |
|
|
||
| /** | ||
| * Get an outgoing share-tracking fragment | ||
| * @return {!Promise<string|undefined>} |
|
Nitpicks, LGTM. |
muxin
force-pushed
the
share-tracking-service
branch
from
d6df2e6
to
af2ebc4
Compare
| user.error(TAG, 'The response from [' + vendorUrl + '] does not ' + | ||
| 'have a fragment value.'); | ||
| return ''; | ||
| }).catch(error => { |
There was a problem hiding this comment.
Nitpick: This callback can be the second param to #then above.
|
LGTM. |
…pproject#4019) * `AmpShareTracking` will get incoming and outgoing share tracking identifier * Get incoming fragment: from url hash directly or ask viewer to get it * Get outgoing fragment: from url endpoint provided by vendor or generate randomly
#3135
AmpShareTrackingwill get incoming and outgoing share tracking identifier