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

Move libraries used for interactions to webpack #10388

Merged
merged 14 commits into from Aug 24, 2020
Merged

Move libraries used for interactions to webpack #10388

merged 14 commits into from Aug 24, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented Aug 19, 2020

Overview

  1. This PR fixes or fixes part of #[fill_in_number_here].
  2. This PR does the following: Move libraries used for interactions using webpack

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 Aug 19, 2020

Hi, @nishantwrp, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

@oppiabot
Copy link

oppiabot bot commented Aug 19, 2020

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

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! A two questions.

import 'third-party-imports/guppy.import';
import 'third-party-imports/midi-js.import';
import 'third-party-imports/ng-audio.import';
import 'third-party-imports/ng-joy-ride.import';
import 'third-party-imports/skulpt.import';
Copy link
Member

@vojtechjelinek vojtechjelinek Aug 20, 2020

Choose a reason for hiding this comment

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

Could any of these newly added libs be only imported at places where they are actually needed? Like in some directive where they are used.

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 20, 2020

Choose a reason for hiding this comment

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

Actually the issue with that is these imports look like this

window.Guppy = ...

and requiring this multiple times loads the library script many times. Plus anyways they need to be loaded in exploration editor and player page. So, added these here to load the script only once.

Copy link
Member

@vojtechjelinek vojtechjelinek Aug 21, 2020

Choose a reason for hiding this comment

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

Ok, that makes sense.

@@ -24,6 +24,5 @@
</oppia-root>
@load('pages/footer_js_libs.html')
<script src="/third_party/static/MathJax-2.7.5/MathJax.js?config=default"></script>
@loadExtensions('interactions/codemirror.html')
Copy link
Member

@vojtechjelinek vojtechjelinek Aug 20, 2020

Choose a reason for hiding this comment

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

Why is it no longer needed here?

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 20, 2020

Choose a reason for hiding this comment

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

its added in the respective directive.

Copy link
Member

@vojtechjelinek vojtechjelinek Aug 21, 2020

Choose a reason for hiding this comment

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

Ok.

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

@vojtechjelinek vojtechjelinek removed their assignment Aug 21, 2020
Copy link
Contributor

@ankita240796 ankita240796 left a comment

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

@oppiabot
Copy link

oppiabot bot commented Aug 21, 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!

Copy link
Member

@DubeySandeep DubeySandeep left a comment

LGTM for the core/templates/pages/contributor-dashboard-page/question-opportunities/question-opportunities.directive.ts file changes!

@DubeySandeep DubeySandeep removed their assignment Aug 21, 2020
Copy link
Member

@kevintab95 kevintab95 left a comment

Codeowner files LGTM, thanks @nishantwrp!

@oppiabot
Copy link

oppiabot bot commented Aug 21, 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!

Copy link
Contributor

@Showtim3 Showtim3 left a comment

LGTM

Copy link
Contributor

@bansalnitish bansalnitish left a comment

LGTM for codeowner file.

@bansalnitish bansalnitish removed their assignment Aug 22, 2020
@Showtim3 Showtim3 removed their assignment Aug 22, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Aug 23, 2020

@U8NWXD @kevinlee12 There are two Travis CI checks in this pr. Any ideas on how to fix this?

@nishantwrp nishantwrp changed the title Move libraries used for interactions using webpack Move libraries used for interactions to webpack Aug 23, 2020
@kevinlee12
Copy link
Contributor

kevinlee12 commented Aug 23, 2020

@nishantwrp Travis does that sometimes, as long as the PR isn't blocked for merge (assuming all reviewers ok), then it's fine.

aks681
aks681 approved these changes Aug 24, 2020
@aks681 aks681 assigned nishantwrp and unassigned aks681 Aug 24, 2020
@nishantwrp nishantwrp merged commit 65a1e80 into oppia:develop Aug 24, 2020
6 checks passed
@nishantwrp nishantwrp deleted the interaction-libs-webpack-final branch Aug 24, 2020
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