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.3.1: Type definitions for guppy, removed blobbuilder #9278

Merged
merged 9 commits into from May 11, 2020
Merged

Milestone 1.3.1: Type definitions for guppy, removed blobbuilder #9278

merged 9 commits into from May 11, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented May 9, 2020

Overview

1. This PR fixes or fixes part of #[fill_in_number_here]. #6351
2. This PR does the following: Type definitions for blobbuilder, guppy

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 9, 2020

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

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Two comments.

// Reference - https://developer.mozilla.org/en-US/docs/Web/API/BlobBuilder

class BlobBuilder {
append: (data: ArrayBuffer) => void;
append: (data: Blob) => void;
append: (data: String, endings?: String) => void;
getBlob: (contentType?: String) => void;
getFile: (name: String, contentType?: String) => File;
}
Copy link
Member

@vojtechjelinek vojtechjelinek May 9, 2020

Choose a reason for hiding this comment

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

Hmmm, I have just looked where we use this and I think we can remove BlobBuilder from our codebase entirely new Blob is supported among all the newer browsers (https://caniuse.com/#feat=blobbuilder), can you try to remove BlobBuilder from our codebase?

Copy link
Contributor Author

@nishantwrp nishantwrp May 9, 2020

Choose a reason for hiding this comment

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

Done!

// @ts-ignore
// Ts Ignore is used here because actually Guppy is a class
// constructor having some properties. Ts doesn't support these
// kind of objects.
Copy link
Member

@vojtechjelinek vojtechjelinek May 9, 2020

Choose a reason for hiding this comment

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

Could you explain this a bit more, maybe add links to some explanation online?

Copy link
Contributor Author

@nishantwrp nishantwrp May 9, 2020

Choose a reason for hiding this comment

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

Ok. Let there be a class SampleClass. Now we can create an object belonging to this class like this.

const sampleObject = new SampleClass();

but there are no properties of a constructor class like.

const a = SampleClass.property;

Guppy has some properties like these like init. So this had to be done.

Copy link
Member

@vojtechjelinek vojtechjelinek May 9, 2020

Choose a reason for hiding this comment

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

So let us rewrite it to something like: Type system does not support constructor properties (Guppy.init) thus we need ts-ignore it.

Copy link
Contributor Author

@nishantwrp nishantwrp May 9, 2020

Choose a reason for hiding this comment

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

Done

@nishantwrp nishantwrp changed the title Type definitions for blobbuilder, guppy Milestone 1.2.1: Type definitions for blobbuilder, guppy May 9, 2020
@nishantwrp nishantwrp changed the title Milestone 1.2.1: Type definitions for blobbuilder, guppy Milestone 1.3.1: Type definitions for blobbuilder, guppy May 9, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 9, 2020

@vojtechjelinek PTAL!

@nishantwrp nishantwrp changed the title Milestone 1.3.1: Type definitions for blobbuilder, guppy Milestone 1.3.1: Type definitions for guppy, removed blobbuilder May 9, 2020
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

@vojtechjelinek
Copy link
Member

vojtechjelinek commented May 9, 2020

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

@aks681 aks681 left a comment

Lgtm for codeowner files.

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

nishantwrp commented May 9, 2020

@kevintab95 Done!

Copy link
Member

@kevintab95 kevintab95 left a comment

LGTM, thanks @nishantwrp!

@oppiabot
Copy link

oppiabot bot commented May 10, 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!

@codecov
Copy link

codecov bot commented May 10, 2020

Codecov Report

Merging #9278 into develop will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #9278      +/-   ##
===========================================
- Coverage    53.54%   53.53%   -0.02%     
===========================================
  Files          875      875              
  Lines        36278    36276       -2     
  Branches      4295     4293       -2     
===========================================
- Hits         19424    19417       -7     
- Misses       15908    15913       +5     
  Partials       946      946              
Flag Coverage Δ
#frontend 53.53% <100.00%> (-0.02%) ⬇️
Impacted Files Coverage Δ
...pia-interactive-math-expression-input.directive.ts 11.27% <ø> (ø)
...e/templates/services/assets-backend-api.service.ts 97.09% <100.00%> (-0.23%) ⬇️
.../topic-editor-page/topic-editor-page.controller.ts 57.14% <0.00%> (-2.86%) ⬇️
...entity-creation-services/topic-creation.service.ts 12.50% <0.00%> (-1.29%) ⬇️
...ions/interactions/interactionsQuestionsRequires.ts 100.00% <0.00%> (ø)

Copy link
Member

@DubeySandeep DubeySandeep left a comment

@nishantwrp, LGTM, just a few questions, PTAL!

@@ -0,0 +1,97 @@
/* eslint-disable camelcase */
// Code - third_party/static/guppy-b5055b/src/guppy.js
Copy link
Member

@DubeySandeep DubeySandeep May 10, 2020

Choose a reason for hiding this comment

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

Wanted to check whether Guppy can also be an auto-upgrade?

Copy link
Contributor Author

@nishantwrp nishantwrp May 10, 2020

Choose a reason for hiding this comment

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

I don't really know what is the proccess of upgrading the libs in manifest.json. Probably @vojtechjelinek can help here.

Also Guppy's repo has been inactive for about 2 years. And we are using the latest version from that repo I guess.

Copy link
Member

@vojtechjelinek vojtechjelinek May 10, 2020

Choose a reason for hiding this comment

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

No, it can't since even a minor version needs to be upgraded manually.

$httpBackend.verifyNoOutstandingExpectation();
});

it('should successfully process a fetch file when Blob throws a TypeError' +
Copy link
Member

@DubeySandeep DubeySandeep May 10, 2020

Choose a reason for hiding this comment

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

Make sure we have another test for the success process.

Copy link
Contributor Author

@nishantwrp nishantwrp May 10, 2020

Choose a reason for hiding this comment

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

I only removed the part where BlobBuilder was involved.

Copy link
Member

@DubeySandeep DubeySandeep May 11, 2020

Choose a reason for hiding this comment

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

Can you please check whether we still have a test that checks the successful creation of Blob out of fetched audio/images data? Maybe share the link to that test block*

[This comment wasn't published earlier]

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.

Here

it('should successfully fetch and cache audio', function() {

@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 11, 2020

@DubeySandeep I think I answered all the reviews.

@DubeySandeep
Copy link
Member

DubeySandeep commented May 11, 2020

@marianazangrossi, can you please check whether it's expected to have an error for frontend test coverage?
image

@kevinlee12
Copy link
Contributor

kevinlee12 commented May 11, 2020

@DubeySandeep that error is fine, we run coverage checks on the frontend suite itself so as long as that passes, it's fine.

@kevinlee12 kevinlee12 merged commit 59fff78 into oppia:develop May 11, 2020
17 of 18 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

9 participants