COOKIE macro #22815
COOKIE macro #22815
Conversation
| * @param {string} name | ||
| * @return {?string} | ||
| */ | ||
| export function cookieReader(win, name) { |
There was a problem hiding this comment.
nit: we could inline this method in variables.js so that we don't need to rename this file. (to avoid the filename conflict with src/cookies.js)
your call.
There was a problem hiding this comment.
I wanted to keep the methods in cookie.js (formerly cookie-writer.js) because I felt they support the same feature, and we are adding more logic to the #cookieReader() method.
However I realized today I can't do this, because cookie.js and variables.js import from each other then 😢
I don't want to introduce a cookie-reader file, so I'm going to move the method to variables.js.
However it seems to me variables.js has a lot of unrelated methods. I'd like to move them around later.
There was a problem hiding this comment.
So I started to move the method to variables.js, and it reminded me there's another reason why I didn't want to do that. We perform privacy check on cookie reading/writing. And I really don't like that the getCookie setCookie function to live everywhere across the code base : (
Any good ideas? If not I'm going to introduce a new cookie-reader file
|
Oh definitely, FIE as well |
|
Ok, this is more difficult than I thought. The FIE and the ampdoc share the same VariableService. There's no way for me to tell if the call is from a FIE or not. Any ideas? |
|
Unfortunately I couldn't think of a good way to solve this. I'll need to move this to |
|
For FIE, we disallow CLIENT_ID in a4a-variouble-source.js . COOKIE here is a bit different though. |
zhouyx
force-pushed
the
cookie-reader
branch
4 times, most recently
from
7f68d72
to
e8308d2
Compare
|
PTAL |
|
integration test added |
build-system/amp4test.js
Outdated
| @@ -186,7 +186,11 @@ app.use('/request-bank/:bid/teardown/', (req, res) => { | |||
| */ | |||
| app.get('/a4a/:bid', (req, res) => { | |||
| const {bid} = req.params; | |||
| const body = ` | |||
| const preSet = ` | |||
There was a problem hiding this comment.
to not introduce custom js in the ad, you can set the cookie via HTTP response header
There was a problem hiding this comment.
Sounds like a better solution. Let me try that
|
Addressed comments. Ready for review. |
| @@ -67,6 +68,7 @@ describe('amp-analytics', function() { | |||
| expect(q['b']).to.equal('AMP TEST'); | |||
| expect(q['cid']).to.equal('amp-12345'); | |||
| expect(q['loadend']).to.not.equal('0'); | |||
| expect(q['cookie']).to.equal('test'); | |||
|
The expander is broken in singlepass. The PR is blocked by #22919 |
* create cookie-reader file * forbid cookie in fie and inabox * new file * add integration test * address comment * add comment
Implements #15654