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

Use '.min' in filenames of already minified js files in the Swagger module so they aren't getting minified again in production, fixes #16927 - for Magento 2.2 #17626

Merged

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Aug 15, 2018

Please: do not forwardport this PR to 2.3, the solution for 2.3 is different and much cleaner, and, it already exists: #17627

Description

The Magento Swagger module contains already minified js files:

This PR marks them to not be minified again during static content deploy, because they get changed in a way that they are no longer valid JS.
This PR renames them so they contain .min and reference them as such from the layout xml file as well. This way they don't get minified again in production mode when minification is enabled.

I've already gone ahead and created a fix for Magento 2.3 as well , since the solution is very different and much cleaner, this is due to #13687 which only exists in 2.3
PR for Magento 2.3: #17627

Fixed Issues (if relevant)

  1. 2.2.5 Swagger: With JS minification enabled, the swagger-ui-bundle.js becomes corrupted #16927: 2.2.5 Swagger: With JS minification enabled, the swagger-ui-bundle.js becomes corrupted

Manual testing scenarios

  1. Set up a clean Magento 2.2.5 installation
  2. Run: bin/magento config:set dev/js/minify_files 1
  3. Run: bin/magento deploy:mode:set production
  4. Go to: https://myinstallation.domain/swagger
  5. See JS errors: Uncaught SyntaxError: Invalid regular expression flag & Uncaught ReferenceError: SwaggerUIBundle is not defined

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 on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @hostep. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@miguelbalparda
Copy link
Contributor

Both codacy and Travis are complaining about the same, care to check @hostep?

…odule so they aren't getting minified again in production, fixes magento#16927 - for Magento 2.2
@hostep hostep force-pushed the fix-issue-16927-for-magento-22 branch from eb924a3 to 154c3ad Compare August 16, 2018 17:30
@hostep hostep changed the title Exclude already minified js files from minification in the Swagger mo… Use '.min' in filenames of already minified js files in the Swagger module so they aren't getting minified again in production, fixes #16927 - for Magento 2.2 Aug 16, 2018
@hostep
Copy link
Contributor Author

hostep commented Aug 16, 2018

@miguelbalparda: I took a completely different approach now, I think it's a better solution and easier to maintain. I've amended my previous commit and force pushed my branch. I also updated the description and title in the opening post so it makes more sense with this new solution. Sorry for the mess! :)

@magento-engcom-team
Copy link
Contributor

Hi @miguelbalparda, thank you for the review.
ENGCOM-2837 has been created to process this Pull Request

@magento-engcom-team magento-engcom-team merged commit 154c3ad into magento:2.2-develop Aug 27, 2018
@magento-engcom-team
Copy link
Contributor

Hi @hostep. Thank you for your contribution.
We will aim to release these changes as part of 2.2.7.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

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

3 participants