Skip to content

[SE-3401] Upgrades Webpack to v4.44.2 to support content hashing - #265

Closed
nizarmah wants to merge 5 commits into
opencraft-release/juniper.3from
nizar/fix_multiple_instances_webpack_juniper_3_backport
Closed

[SE-3401] Upgrades Webpack to v4.44.2 to support content hashing#265
nizarmah wants to merge 5 commits into
opencraft-release/juniper.3from
nizar/fix_multiple_instances_webpack_juniper_3_backport

Conversation

@nizarmah

@nizarmah nizarmah commented Sep 30, 2020

Copy link
Copy Markdown

Upgrades Webpack from v2.7.0 to v4.44.2 so that content hashing is possible.

JIRA tickets: SE-3401

Sandbox URL:

Testing instructions:

  1. Spawn multiple instances and add to load-balancer.
  2. Open the Developer console for the LMS and Studio and verify there are no 404 messages for commons.js.
  3. SSH into the different instances and verify that the output for ls /edx/var/edxapp/staticfiles/studio/bundles/ | grep -i commons is the same among the different instances.
  4. Compare the files in /edx/var/edxapp/staticfiles/studio/bundles/ and /edx/var/edxapp/staticfiles/bundles/ between both App Servers and make sure they have the same filename (including hashes).
  5. Compare the files in /edx/var/edxapp/staticfiles/studio/bundles/ and /edx/var/edxapp/staticfiles/bundles/ between app servers with old instances (without the fix) and new ones (with the fix) and make sure everything is similar/acting normal.
  6. Verify that the assets are getting loaded correctly.

Reviewers

@gabor-boros gabor-boros left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 🎉

  • I tested this: Tested asset loading from a normal window
  • I tested this: Tested asset loading from a private window
  • I tested this: Tested asset loading from a tor window
  • I tested this: Tested asset checksum match on the instances
  • I read through the code
    - [ ] NA I checked for accessibility issues
    - [ ] NA Includes documentation
    - [ ] NA I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@gabor-boros gabor-boros left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This solves the issue for sure, but the asset is not compressed anymore, which will result in higher load time

@nizarmah

Copy link
Copy Markdown
Author

This solves the issue for sure, but the asset is not compressed anymore, which will result in higher load time

I totally missed that part. Sorry about that! Thanks for paying attention to that!

@nizarmah
nizarmah force-pushed the nizar/fix_multiple_instances_webpack_juniper_3_backport branch 2 times, most recently from f2d6e7e to d2dba5c Compare September 30, 2020 11:55
@nizarmah
nizarmah marked this pull request as draft September 30, 2020 13:10
@nizarmah

nizarmah commented Oct 1, 2020

Copy link
Copy Markdown
Author

@gabor-boros

This solves the issue for sure, but the asset is not compressed anymore, which will result in higher load time

I compared the file to other instances; I also compared the loading time of the file with other instances. And I can't relate to what you are mentioning. The asset seems to be as "compressed" as it already were. In addition, the loading time is closely similar between different instances.

The cherry-on-top is that this file is the same between the LMS and Studio. So by content-hashing it, it gets cached the first time it loads. Meanwhile, on other instances, the file has to be downloaded on LMS and Studio so that it is cached for each (since it is chunk hashed).

Can you confirm what I said, and compare the file on different instances, so that I can mark this as "Ready for Review" please?

Side note: the commons.js can be found at: /edx/app/edxapp/edx-platform/common/static/bundles/commons.js
It should be almost identical with LMS and Studio from other instances, but it shouldn't have the list of hashes they do.

@gabor-boros
gabor-boros marked this pull request as ready for review October 1, 2020 05:56

@gabor-boros gabor-boros left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 🎉

  • I tested this: Tested asset loading from a normal window
  • I tested this: Tested asset loading from a private window
  • I tested this: Tested asset loading from a tor window
  • I tested this: Tested asset checksum match on the instances
  • I read through the code
    - [ ] NA I checked for accessibility issues
    - [ ] NA Includes documentation
    - [ ] NA I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

Ok, I have to apologize here. It turned out that the output I've seen seems compressed, but a "cool new feature" (which is really good but should be promoted), tricked me. The screenshot below is one line, but displayed as a "formatted" string...

In reality that's just one line. Sorry for the previous.

Screenshot 2020-10-01 at 7 50 07

@nizarmah

nizarmah commented Oct 1, 2020

Copy link
Copy Markdown
Author

Hahaha no worries 😄 Thanks for the review 👍 and for making sure what I mentioned was correct 🙂

I'll try to search for a core member to review this.

@nizarmah
nizarmah force-pushed the nizar/fix_multiple_instances_webpack_juniper_3_backport branch from 359109a to 13135b5 Compare October 1, 2020 07:33
@nizarmah
nizarmah requested a review from lgp171188 October 1, 2020 09:59
@nizarmah
nizarmah force-pushed the nizar/fix_multiple_instances_webpack_juniper_3_backport branch 3 times, most recently from 8a082d8 to dd4c89a Compare October 2, 2020 16:35
@nizarmah
nizarmah requested a review from gabor-boros October 2, 2020 16:35
@nizarmah nizarmah changed the title [SE-3401] Fixes webpack hashes to be content specific to support multiple instances [SE-3401] Fixes webpack hashes to be content specific to be consistent among multiple instances Oct 2, 2020
@nizarmah nizarmah changed the title [SE-3401] Fixes webpack hashes to be content specific to be consistent among multiple instances [SE-3401] Webpack Hashes to use Content Hash to be consistent among multiple instances Oct 3, 2020
@nizarmah
nizarmah requested review from pkulkark and removed request for lgp171188 October 5, 2020 08:53
@nizarmah
nizarmah force-pushed the nizar/fix_multiple_instances_webpack_juniper_3_backport branch 5 times, most recently from ab82f44 to d51c620 Compare October 5, 2020 15:07
@nizarmah
nizarmah force-pushed the nizar/fix_multiple_instances_webpack_juniper_3_backport branch 11 times, most recently from 13f6b0b to f12a0ed Compare October 5, 2020 20:01
@nizarmah
nizarmah force-pushed the nizar/fix_multiple_instances_webpack_juniper_3_backport branch 3 times, most recently from e9f5733 to 7473c3e Compare October 6, 2020 00:19
@nizarmah nizarmah changed the title [SE-3401] Webpack Hashes to use Content Hash to be consistent among multiple instances [SE-3401] Upgrades Webpack to v4.44.2 to support content hashing Oct 6, 2020
@nizarmah

nizarmah commented Nov 18, 2020

Copy link
Copy Markdown
Author

I'm closing this PR since it's no longer necessary. And I'll work on this contribution during my own personal time 👍 https://github.com/edx/edx-platform/pull/25635

@nizarmah nizarmah closed this Nov 18, 2020
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.

2 participants