Import wave-surfer using webpack instead of including it in third_party.js file #8922

nishantwrp · 2020-03-24T22:33:33Z

Overview

This PR does the following: Imports wave-surfer using webpack instead of third_party_js.

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.

…ty.js file

oppiabot · 2020-03-24T22:33:35Z

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

kevintab95

Just one minor comment. PTAL @nishantwrp.

kevintab95 · 2020-03-24T22:36:55Z

core/templates/third-party-imports/wave-surfer.scripts.ts

@@ -0,0 +1,2 @@
+window.WaveSurfer = require(
+  '../../../third_party/static/wave-surfer-js-2.2.1/wavesurfer.min.js');


Relative imports are not allowed. The lint check for this doesn't work properly but #8870 should fix it. But meanwhile, can you please correct this instance? Thanks!

@kevintab95 Fixed this.

kevinlee12

lgtm for .github/CODEOWNERS only.

vojtechjelinek

Thanks! Two comments.

vojtechjelinek · 2020-03-25T14:53:51Z

...oration-editor-page/translation-tab/audio-translation-bar/audio-translation-bar.directive.ts

@@ -48,6 +48,7 @@ require('services/context.service.ts');
 require('services/editability.service.ts');
 require('services/id-generation.service.ts');
 require('services/user.service.ts');
+require('third-party-imports/wave-surfer.scripts.ts');


rename this to wave-surfer.import.ts

also, does it make sense to load the variable into window.WaveSurfer? Can we just load it to a local variable named WaveSurfer here and use it?

@vojtechjelinek I think in my GSoC proposal we agreed to follow *scripts.ts naming?

I think that the final conclusion was to use *.import.ts for all the files that only contain requires/imports. I may have made a mistake in some of my comments. But I think that the import naming is much more decriptive.

@vojtechjelinek Renamed the file. But had a doubt about your second comment.

Are you saying to modify the wave-surfer.import.ts to something like

module.exports = require( 'static/wave-surfer-js-2.2.1/wavesurfer.min.js');

and modifying the require statement in audio-translation-bar.directive.ts to something like

var WaveSurfer = require('third-party-imports/wave-surfer.import.ts');

and use that in that file?

In my opinion, I think it looks weird we don't assign a variable to require statements in any other file. I think we should do that work in .import.ts file itself. What do you think?

We actually use this for example for the hashes already

oppia/core/templates/domain/utilities/url-interpolation.service.spec.ts

Line 28 in ff31fc3

let hashes = require('hashes.json');

from my POV it doesn't make sense to make the library available for the whole window namespace if it is required just in the one file.

Ok, Makes sense. I'll update the code.

@vojtechjelinek Done! Also removed the custom declarations as they are no longer needed.

codecov · 2020-03-25T20:56:35Z

Codecov Report

Merging #8922 into develop will increase coverage by <.01%.
The diff coverage is 66.67%.

@@             Coverage Diff             @@
##           develop    #8922      +/-   ##
===========================================
+ Coverage    53.06%   53.06%   +<.01%     
===========================================
  Files          862      863       +1     
  Lines        35502    35504       +2     
  Branches      4212     4212              
===========================================
+ Hits         18838    18840       +2     
  Misses       15746    15746              
  Partials       918      918

Flag	Coverage Δ
#frontend	`53.06% <66.67%> (ø)`	⬆️

Impacted Files	Coverage Δ
...emplates/third-party-imports/wave-surfer.import.ts	`100% <100%> (ø)`
...translation-bar/audio-translation-bar.directive.ts	`6.21% <50%> (+0.29%)`	⬆️

kevintab95

Code-owner file LGTM, thanks @nishantwrp!

vojtechjelinek

Thanks! Just one small comment.

vojtechjelinek · 2020-03-26T12:00:47Z

...oration-editor-page/translation-tab/audio-translation-bar/audio-translation-bar.directive.ts

@@ -48,6 +48,7 @@ require('services/context.service.ts');
 require('services/editability.service.ts');
 require('services/id-generation.service.ts');
 require('services/user.service.ts');
+var WaveSurfer = require('third-party-imports/wave-surfer.import.ts');


maybe use const instead of var

ankita240796

LGTM from code-owner's perspective, Thanks @nishantwrp!

…rty.js file (oppia#8922) * Import wave-surfer using webpack instead of including it in third_party.js file * Codeowners * Removed Relative Imports * Add file to coverage tests * Load WaveSurfer in only one file * var -> const

nishantwrp added 2 commits March 25, 2020 03:43

Import wave-surfer using webpack instead of including it in third_par…

656a0d2

…ty.js file

Codeowners

08a055b

nishantwrp added the PR CHANGELOG: Speed improvement -- @vojtechjelinek label Mar 24, 2020

nishantwrp requested review from aks681, kevinlee12, kevintab95 and vojtechjelinek as code owners March 24, 2020 22:33

oppiabot bot assigned vojtechjelinek Mar 24, 2020

kevintab95 requested changes Mar 24, 2020

View reviewed changes

kevinlee12 approved these changes Mar 25, 2020

View reviewed changes

kevinlee12 assigned nishantwrp Mar 25, 2020

vojtechjelinek suggested changes Mar 25, 2020

View reviewed changes

nishantwrp added 2 commits March 26, 2020 01:56

Removed Relative Imports

bd18c3a

Add file to coverage tests

25388f8

nishantwrp requested review from ankita240796 and marianadasilvadev as code owners March 25, 2020 20:31

nishantwrp requested review from vojtechjelinek and kevintab95 March 25, 2020 20:37

nishantwrp removed their assignment Mar 25, 2020

Load WaveSurfer in only one file

0a236c2

nishantwrp closed this Mar 26, 2020

nishantwrp reopened this Mar 26, 2020

kevintab95 approved these changes Mar 26, 2020

View reviewed changes

vojtechjelinek approved these changes Mar 26, 2020

View reviewed changes

var -> const

a35bcdc

vojtechjelinek assigned aks681 and ankita240796 and unassigned vojtechjelinek Mar 26, 2020

vojtechjelinek assigned marianadasilvadev Mar 26, 2020

ankita240796 approved these changes Mar 26, 2020

View reviewed changes

ankita240796 removed their assignment Mar 26, 2020

kevinlee12 merged commit fce21b2 into oppia:develop Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import wave-surfer using webpack instead of including it in third_party.js file #8922

Import wave-surfer using webpack instead of including it in third_party.js file #8922

nishantwrp commented Mar 24, 2020

oppiabot bot commented Mar 24, 2020

kevintab95 left a comment

kevintab95 Mar 24, 2020

nishantwrp Mar 25, 2020

kevinlee12 left a comment

vojtechjelinek left a comment

vojtechjelinek Mar 25, 2020

vojtechjelinek Mar 25, 2020

nishantwrp Mar 25, 2020

vojtechjelinek Mar 25, 2020

nishantwrp Mar 25, 2020

vojtechjelinek Mar 25, 2020

nishantwrp Mar 25, 2020

nishantwrp Mar 25, 2020

codecov bot commented Mar 25, 2020 •

edited

Loading

kevintab95 left a comment

vojtechjelinek left a comment

vojtechjelinek Mar 26, 2020

nishantwrp Mar 26, 2020

ankita240796 left a comment

		@@ -0,0 +1,2 @@
		window.WaveSurfer = require(
		'../../../third_party/static/wave-surfer-js-2.2.1/wavesurfer.min.js');

Import wave-surfer using webpack instead of including it in third_party.js file #8922

Import wave-surfer using webpack instead of including it in third_party.js file #8922

Conversation

nishantwrp commented Mar 24, 2020

Overview

Essential Checklist

PR Pointers

oppiabot bot commented Mar 24, 2020

kevintab95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinlee12 left a comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 25, 2020 • edited Loading

Codecov Report

kevintab95 left a comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankita240796 left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 25, 2020 •

edited

Loading