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.

Sign up for GitHub

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

Jump to bottom

Initialize inline translations module only when they are enabled #27696

Merged
merged 13 commits into from
Aug 26, 2020
Merged

Initialize inline translations module only when they are enabled #27696

merged 13 commits into from
Aug 26, 2020

Conversation

krzksz
Copy link
Contributor

@krzksz krzksz commented Apr 10, 2020 •
edited by m2-assistant bot
Loading

Description (*)

This PR is a part of my effort to improve Magento 2 frontend performance.

The focus point of this PR was to remove downloading of inline translations JavaScript modules when they are not enabled. This was happening because they were defined in deps, which is completely redundant because they are properly initialized in https://github.com/krzksz/magento2/blob/98e04e174906799b941011a9fb1b5890a5e7cae9/app/code/Magento/Translation/view/frontend/templates/translate_inline.phtml#L57

While I was touching the files anyway I took the liberty to:

  • Move escapeHTML to utilities (it is not used anywhere else and there is already an unescape counterpart).
  • Fixed utilities dependency for serialize method.
  • Simplify logic and variables in few places.

Performance improvement

Tested on Magento 2.4-develop with Luma and sample data.

  • 207 → 203 JS requests (-1.93%).
  • 794.39kB → 773.44kB JS transferred (-2.64%).

Manual testing scenarios (*)

  1. When inline translations are disabled there is no request for translate-inline JS file.
  2. When inline translations are enabled, they work as expected.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Initialize inline translations module only when they are enabled #29553: Initialize inline translations module only when they are enabled

@krzksz krzksz added the Area: Perf/Frontend All tickets related with improving frontend performance. label Apr 10, 2020
@m2-assistant
Copy link

m2-assistant bot commented Apr 10, 2020

Hi @krzksz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@krzksz krzksz requested a review from Bartlomiejsz April 10, 2020 15:50
@krzksz krzksz marked this pull request as draft April 19, 2020 21:26
@krzksz
Copy link
Contributor Author

krzksz commented Apr 21, 2020

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @krzksz. Thank you for your request. I'm working on Magento instance for you

@krzksz
Copy link
Contributor Author

krzksz commented Apr 21, 2020

@magento give me 2.4-develop instance

@magento-engcom-team
Copy link
Contributor

Hi @krzksz. Thank you for your request. I'm working on Magento 2.4-develop instance for you

@magento-engcom-team
Copy link
Contributor

Hi @krzksz, here is your new Magento instance.
Admin access: https://pr-27696.instances.magento-community.engineering/admin_370d
Login: f761e507 Password: 3eb2cb42c5e1
Instance will be terminated in up to 3 hours.

@krzksz
Copy link
Contributor Author

krzksz commented Apr 21, 2020

@magento run all tests

1 similar comment
@krzksz
Copy link
Contributor Author

krzksz commented May 20, 2020

@magento run all tests

@krzksz
Copy link
Contributor Author

krzksz commented May 21, 2020

@magento run all tests

@krzksz
Copy link
Contributor Author

krzksz commented May 24, 2020

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @krzksz. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @krzksz, here is your new Magento instance.
Admin access: https://pr-27696.instances.magento-community.engineering/admin_c5e1
Login: 7370547e Password: b01752ce655a
Instance will be terminated in up to 3 hours.

@krzksz
Copy link
Contributor Author

krzksz commented May 24, 2020

@magento give me 2.4-develop instance

@ihor-sviziev ihor-sviziev removed their assignment Aug 15, 2020
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ihor-sviziev ihor-sviziev self-assigned this Aug 15, 2020
@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Aug 17, 2020
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8018 has been created to process this Pull Request

@engcom-Alfa engcom-Alfa self-assigned this Aug 17, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Tested on Magento 2.4-develop with Luma and sample data.

205 → 201 JS requests (-1.96%).
794kB → 779kB JS transferred (-1.89%).

When inline translations are disabled there is no request for translate-inline JS file.
When inline translations are enabled, they work as expected.

Was tested and on the Blank theme.

@engcom-Charlie engcom-Charlie self-assigned this Aug 17, 2020
@engcom-Charlie engcom-Charlie added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label Aug 17, 2020
magento-engcom-team pushed a commit that referenced this pull request Aug 26, 2020
@sidolov
ENGCOM-8018: Initialize inline translations module only when they are…
c4d2500 
… enabled #27696
@magento-engcom-team magento-engcom-team merged commit e5f88ed into magento:2.4-develop Aug 26, 2020
@m2-assistant
Copy link

m2-assistant bot commented Aug 26, 2020

Hi @krzksz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

@Bartlomiejsz Bartlomiejsz Awaiting requested review from Bartlomiejsz

@adifucan adifucan Awaiting requested review from adifucan

Assignees

@ihor-sviziev ihor-sviziev

@adifucan adifucan

@engcom-Alfa engcom-Alfa

@engcom-Charlie engcom-Charlie

Labels
Area: Frontend Area: Lib/Frontend Area: Perf/Frontend All tickets related with improving frontend performance. Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Award: bug fix Award: category of expertise Component: Catalog Component: Translation Partner: creativestyle partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
Pull Requests Dashboard
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Issue] Initialize inline translations module only when they are enabled
7 participants
@krzksz @magento-engcom-team @ihor-sviziev @sidolov @engcom-Alfa @adifucan @engcom-Charlie