-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update autoconsent #101
Update autoconsent #101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, I left some small questions.
@@ -186,14 +186,15 @@ class CMPCollector extends BaseCollector { | |||
this.receivedMsgs.push(msg); | |||
switch (msg.type) { | |||
case 'init': { | |||
/** @type {AutoconsentConfig} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about <Partial>
, but doesn't it mean that we DON'T have to include isMainWorld: false
below? Or is the default true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you are right, it would work without it, but I just wanted to make it explicit.
package.json
Outdated
@@ -7,7 +7,7 @@ | |||
"main": "main.js", | |||
"scripts": { | |||
"crawl": "node ./cli/crawl-cli", | |||
"test": "npm run lint && tsc && npm run unit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, doesn't this remove typechecking? I think we want typechecking. Or is this run somehow via lint now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I was sure typescript-eslint did typecheking, but apparently it needs another config. I tried to enable it, but got a bunch of new errors (our code is messy), so I added tsc back here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Max!
After bumping autoconsent version,
tsc
started giving weird lint errors inside the autoconsent folder. Excluding paths didn't work, so I had to change the way we lint too