8000 feat: Convert fns module to TS by yavorona · Pull Request #548 · optimizely/javascript-sdk · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 8 commits into from
Aug 31, 2020
Merged

Conversation

yavorona
Copy link
Contributor
@yavorona yavorona commented Aug 1, 2020

Summary

  • Convert fns module from JS to TS.

Test plan

  • Existing unit tests + running with local bundle

@yavorona yavorona added the WIP label Aug 1, 2020
@yavorona yavorona requested a review from a team as a code owner August 1, 2020 02:20
@yavorona yavorona self-assigned this Aug 1, 2020
@yavorona yavorona changed the title feat: Convert fns module to TS feat: Convert fns module to TS (WIP) Aug 1, 2020
@yavorona yavorona removed the WIP label Aug 4, 2020
@yavorona yavorona removed their assignment Aug 4, 2020
@coveralls
Copy link
coveralls commented Aug 4, 2020

Coverage Status

Coverage decreased (-0.0004%) to 96.773% when pulling 3f0ac54 on pnguen/convert-fns-module-to-ts into dafdb60 on master.

@@ -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';
Copy link
Contributor Author
@yavorona yavorona Aug 17, 2020

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? 🥺

Copy link
Contributor Author

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.

@yavorona yavorona requested a review from mjc1283 August 17, 2020 20:34
@yavorona yavorona removed their assignment Aug 17, 2020
@yavorona yavorona changed the title feat: Convert fns module to TS (WIP) feat: Convert fns module to TS Aug 17, 2020
Copy link
Contributor
@mjc1283 mjc1283 left a 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 {
Copy link
Contributor

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.

@yavorona yavorona force-pushed the pnguen/convert-fns-module-to-ts branch from 34a8c7e to e07e6ed Compare August 19, 2020 20:38

export var assign = function (target) {
// eslint-disable-next-line
export function assign(target: any, ...sources: any[]): any {
Copy link
Contributor Author

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.

@yavorona yavorona removed their assignment Aug 26, 2020
@yavorona yavorona force-pushed the pnguen/convert-fns-module-to-ts branch from 990d06c to b48acf9 Compare August 26, 2020 18:45
@yavorona yavorona closed this Aug 26, 2020
@yavorona yavorona reopened this Aug 26, 2020
@yavorona yavorona removed their assignment Aug 26, 2020
@yavorona yavorona force-pushed the pnguen/convert-fns-module-to-ts branch 2 times, most recently from ce4e5bf to b071757 Compare August 26, 2020 21:14
@yavorona yavorona closed this Aug 27, 2020
@yavorona yavorona reopened this Aug 27, 2020
@yavorona yavorona removed their assignment Aug 27, 2020
@yavorona yavorona requested a review from mjc1283 August 27, 2020 22:50
Copy link
Contributor
@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yavorona yavorona merged commit 4a46cc9 into master Aug 31, 2020
@yavorona yavorona deleted the pnguen/convert-fns-module-to-ts branch August 31, 2020 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0