-
Notifications
You must be signed in to change notification settings - Fork 4
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
[Hackathon] Typescript (Only types, no refactorings) #80
Conversation
This pull request introduces 2 alerts when merging 498f381 into 8f9d752 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging a5a52be into 8f9d752 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 367b3b2 into 8f9d752 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 28e3d98 into 8f9d752 - view on LGTM.com new alerts:
|
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.
Some comments otherwise it looks good to me
Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com>
Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com>
src/pixel/stringify.ts
Outdated
@@ -3,6 +3,6 @@ import { trim } from '../utils/types' | |||
|
|||
export const MASK = '*********' | |||
|
|||
export function replacer (key, value) { | |||
export function replacer (key, value: string): string { |
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.
Is this syntax for key: string, value: string
? Or is there a reason why key doesn't have a type (even any
)?
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.
Default type without any annotation is any. Added it explicitly for clarity
src/utils/page.ts
Outdated
* @return {string|undefined} | ||
*/ | ||
export function getReferrer (win = window) { | ||
export function getReferrer (win = window): string | undefined { |
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.
Maybe missing a type annotation for win
? Same below
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.
updated 👍
asQuery (): IQuery, | ||
} | ||
|
||
export interface IPixelSender { |
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.
Just curiosity: what's the meaning of the I
prefixing a lot of the types?
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.
different than scala, typescript seems to use the same namespace for values and types. As we already have functions with name PixelSender I wanted a type that does not conflict with it.
We should clean this up / refactor this in later prs, but I wanted to keep this one simple
This pull request introduces 1 alert when merging b5587a7 into 8f9d752 - view on LGTM.com new alerts:
|
* [Hackathon] Typescript (Only types, no refactorings) (#80) * migrate build * try wdio * fix rollup * use terser instead of uglify * update wdio config * update wdio to transpile * update preambled.ts * type everything * fix build * fixed types.ts eslint issues * nycrc * Update src/utils/types.ts Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com> * Update src/utils/types.ts Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com> * comments * wip * publish javascript lib and types Co-authored-by: Vinod M <vinodmuralidharan@yahoo.com> Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com> * [No ticket] Update publishing setup (#84) * finish merge master * fix tests * Update .eslintrc.js Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com> * Update .eslintrc.js Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com> * Update src/events/event-bus.ts Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com> * comments * comments * update files in package.json * expose consts Co-authored-by: Vinod M <vinodmuralidharan@yahoo.com> Co-authored-by: Wiem Zine El Abidine <welabidine@liveintent.com>
Short description if any.
Author Todo List:
Ready For Review
status