Skip to content
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

Milestone 1.4.1: Remove some any types from typings directory #9293

Merged
merged 9 commits into from May 15, 2020
Merged

Milestone 1.4.1: Remove some any types from typings directory #9293

merged 9 commits into from May 15, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented May 11, 2020

Overview

1. This PR fixes or fixes part of #[fill_in_number_here].
2. This PR does the following: Remove some any types from typings directory

Essential Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

PR Pointers

  • Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays
  • Never force push. If you do, your PR will be closed.

@oppiabot
Copy link

oppiabot bot commented May 11, 2020

Assigning @kevinlee12 for the first-pass review of this pull request. Thanks!

@@ -49,8 +49,8 @@ export class BrowserCheckerService {
}

private _isMobileDevice(): boolean {
var userAgent = navigator.userAgent || this.windowRef.nativeWindow.opera;
return userAgent.match(/iPhone/i) || userAgent.match(/Android/i);
var userAgent = navigator.userAgent;
Copy link
Contributor Author

@nishantwrp nishantwrp May 11, 2020

Choose a reason for hiding this comment

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

Hi @tjiang11,
Please review the changes. Was opera really needed here?

@@ -1,4 +1,5 @@
window.MathJax = {
// @ts-ignore
Copy link
Contributor Author

@nishantwrp nishantwrp May 11, 2020

Choose a reason for hiding this comment

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

Did this because technically the type error thrown here was correct. This is not the type of MathJax but MathJaxConfig. To not do this we would have to declare the custom mathjax config the other way suggested here.

Copy link
Contributor

@ankita240796 ankita240796 May 12, 2020

Choose a reason for hiding this comment

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

Any specific reason for why can't we follow the custom declaration?

Copy link
Contributor Author

@nishantwrp nishantwrp May 12, 2020

Choose a reason for hiding this comment

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

Yup, MathJax is a namespace itself. Also if we make the type of window.MathJax equal to MathJax.Config it finally becomes MathJax.Config & typeof MathJax. And I think it makes sense because technically window.MathJax should be the MathJax namespace, it shouldn't be just the config, right?

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! few comments.

var userAgent = navigator.userAgent || this.windowRef.nativeWindow.opera;
return userAgent.match(/iPhone/i) || userAgent.match(/Android/i);
var userAgent = navigator.userAgent;
return !!(userAgent.match(/iPhone/i)) || !!(userAgent.match(/Android/i));
Copy link
Member

@vojtechjelinek vojtechjelinek May 11, 2020

Choose a reason for hiding this comment

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

Use Boolean(…) isntead of !!.

Copy link
Contributor Author

@nishantwrp nishantwrp May 11, 2020

Choose a reason for hiding this comment

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

Done

@@ -1,4 +1,5 @@
window.MathJax = {
// @ts-ignore
Copy link
Member

@vojtechjelinek vojtechjelinek May 11, 2020

Choose a reason for hiding this comment

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

Why is this ignored? Add comment explaining this.

Copy link
Contributor Author

@nishantwrp nishantwrp May 11, 2020

Choose a reason for hiding this comment

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

Added a comment in this thread itself. Can you take a look at that.

Copy link
Member

@vojtechjelinek vojtechjelinek May 11, 2020

Choose a reason for hiding this comment

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

Saw but still, this should be explained in the code.

Copy link
Contributor Author

@nishantwrp nishantwrp May 11, 2020

Choose a reason for hiding this comment

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

Done

writable: false
});

window.CodeMirror = require('static/code-mirror-5.17.0/lib/codemirror.js');
Copy link
Member

@vojtechjelinek vojtechjelinek May 11, 2020

Choose a reason for hiding this comment

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

The previous behavior is a but different am I right?

Copy link
Contributor Author

@nishantwrp nishantwrp May 11, 2020

Choose a reason for hiding this comment

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

Maybe. But the Merge View was working fine!

Math?: any;
MathJax?: any;
CodeMirror?: typeof CodeMirror;
HTMLElement?: HTMLElement;
__fixtures__?: any;
Copy link
Member

@vojtechjelinek vojtechjelinek May 11, 2020

Choose a reason for hiding this comment

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

This won't be replaced?

Copy link
Contributor Author

@nishantwrp nishantwrp May 11, 2020

Choose a reason for hiding this comment

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

Will replace it in my next PR.

@@ -8405,7 +8405,7 @@ watchpack@^1.6.1:
graceful-fs "^4.1.2"
neo-async "^2.5.0"

wavesurfer.js@^3.3.3:
wavesurfer.js@3.3.3:
Copy link
Member

@vojtechjelinek vojtechjelinek May 11, 2020

Choose a reason for hiding this comment

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

This is a manual change?

Copy link
Contributor Author

@nishantwrp nishantwrp May 11, 2020

Choose a reason for hiding this comment

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

Nope. Forgot to runyarn install when changed ^3.3.3 to 3.3.3.

@nishantwrp nishantwrp requested a review from vojtechjelinek May 11, 2020
@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #9293 into develop will not change coverage.
The diff coverage is 80.00%.

@@           Coverage Diff            @@
##           develop    #9293   +/-   ##
========================================
  Coverage    53.98%   53.98%           
========================================
  Files          875      875           
  Lines        36176    36176           
  Branches      4312     4311    -1     
========================================
  Hits         19528    19528           
  Misses       15692    15692           
  Partials       956      956           
Flag Coverage Δ
#frontend 53.98% <80.00%> (ø)
Impacted Files Coverage Δ
core/templates/mathjaxConfig.ts 100.00% <ø> (ø)
...plates/pages/signup-page/signup-page.controller.ts 57.14% <0.00%> (ø)
...plates/domain/utilities/browser-checker.service.ts 91.30% <100.00%> (+0.40%) ⬆️
extensions/interactions/codemirrorRequires.ts 100.00% <100.00%> (ø)

Copy link
Contributor

@ankita240796 ankita240796 left a comment

Hi @nishantwrp, Thanks for the PR. A few comments, PTAL.

@@ -18,6 +18,18 @@

import { DateTimeFormatService } from 'services/date-time-format.service';

// Needed to avoid typescript errors.
Copy link
Contributor

@ankita240796 ankita240796 May 12, 2020

Choose a reason for hiding this comment

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

Elaborate this comment more (i.e. explain the exact issue).

Copy link
Contributor Author

@nishantwrp nishantwrp May 12, 2020

Choose a reason for hiding this comment

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

Done!

if (millisSinceEpoch === 0) {
return new OldDate(NOW_MILLIS);
} else {
return new OldDate(millisSinceEpoch);
}
});
} as any as MockDateContructorType;
Copy link
Contributor

@ankita240796 ankita240796 May 12, 2020

Choose a reason for hiding this comment

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

Why do we need as any here?

Copy link
Contributor Author

@nishantwrp nishantwrp May 12, 2020

Choose a reason for hiding this comment

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

Because if you look at the type we declared i.e. MockDateConstructorType, you'll see we have defined some properties of the function itself. So first we have to do as any before assigning that interface to the function because those properties don't really exist in our function.

Reference - https://stackoverflow.com/questions/46501911/typescript-declaring-a-function-with-properties

Copy link
Contributor

@ankita240796 ankita240796 May 13, 2020

Choose a reason for hiding this comment

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

Please add a comment in the code as well.

Copy link
Contributor Author

@nishantwrp nishantwrp May 14, 2020

Choose a reason for hiding this comment

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

Done!

@@ -1,4 +1,5 @@
window.MathJax = {
// @ts-ignore
Copy link
Contributor

@ankita240796 ankita240796 May 12, 2020

Choose a reason for hiding this comment

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

Any specific reason for why can't we follow the custom declaration?

@ankita240796 ankita240796 removed their assignment May 12, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 13, 2020

@vojtechjelinek ping.

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 13, 2020

@ankita240796 Ping!

Copy link
Contributor

@ankita240796 ankita240796 left a comment

LGTM, Thanks @nishantwrp!

if (millisSinceEpoch === 0) {
return new OldDate(NOW_MILLIS);
} else {
return new OldDate(millisSinceEpoch);
}
});
} as any as MockDateContructorType;
Copy link
Contributor

@ankita240796 ankita240796 May 13, 2020

Choose a reason for hiding this comment

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

Please add a comment in the code as well.

@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 14, 2020

Copy link
Contributor

@jameesjohn jameesjohn left a comment

LGTM!
Thanks, @nishantwrp.

@oppiabot
Copy link

oppiabot bot commented May 14, 2020

Hi @nishantwrp. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

@kevinlee12
Copy link
Contributor

kevinlee12 commented May 14, 2020

Canceled TravisCI since there's a merge conflict.

Copy link
Contributor

@kevinlee12 kevinlee12 left a comment

lgtm for code owner files.

@kevinlee12 kevinlee12 removed their assignment May 14, 2020
Copy link
Contributor

@bansalnitish bansalnitish left a comment

LGTM!

aks681
aks681 approved these changes May 15, 2020
Copy link
Member

@aks681 aks681 left a comment

Lgtm as codeowner.

@aks681 aks681 removed their assignment May 15, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 15, 2020

@kevintab95 PTAL!

Copy link
Member

@kevintab95 kevintab95 left a comment

LGTM, thanks @nishantwrp!

@kevintab95 kevintab95 removed their assignment May 15, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 15, 2020

@vojtechjelinek I Think this can be merged now.

@vojtechjelinek vojtechjelinek merged commit 37e46f7 into oppia:develop May 15, 2020
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants