-
Notifications
You must be signed in to change notification settings - Fork 83
feat: Convert fns module to TS #548
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
Conversation
4f8b498
to
e80a843
Compare
@@ -15,20 +15,23 @@ | |||
*/ | |||
import { generateUUID as uuid, keyBy as keyByUtil } from '@optimizely/js-sdk-utils'; | |||
|
|||
var MAX_SAFE_INTEGER_LIMIT = Math.pow(2, 53); | |||
import { C 10000 onfig } from '@optimizely/optimizely-sdk'; |
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.
I believe this should be the right way of importing Config
interface as it compiles successfully with npm run build
. However, the TS compiler is failing with Cannot find module '@optimizely/optimizely-sdk'
error, not sure why. Any ideas @mjc1283? 🥺
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.
This is now outdated. I changed this to type any
after discussing with @mikeng13 . Please see the most recent commit for reference.
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.
Overall LGTM, just wondering if we can do better for assign
.
export var assign = function (target) { | ||
if (!target) { | ||
// eslint-disable-next-line | ||
export function assign(...args: [any]): 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.
Is there a better type signature we can use here? The type of Object.assign
is assign<T, U>(target: T, source: U): T & U
.
34a8c7e
to
e07e6ed
Compare
|
||
export var assign = function (target) { | ||
// eslint-disable-next-line | ||
export function assign(target: any, ...sources: any[]): 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.
@mjc1283 let me know if that works. I assumed we still want to go with any
is this case.
990d06c
to
b48acf9
Compare
ce4e5bf
to
b071757
Compare
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
Summary
fns
module from JS to TS.Test plan