tedious / JShrink

Javascript Minifier built in PHP
http://www.tedivm.com
BSD 3-Clause "New" or "Revised" License
751 stars 152 forks source link

jquery/jquery.validate.min.js file isn't getting minified properly. #110

Open CGLOB opened 1 year ago

CGLOB commented 1 year ago

[ pub/static/frontend/Magento/luma/en_US/jquery/jquery.validate.min.js ] is partially minified. It still contains parts without minifying.

Ex: If you scroll to the bottom, you will see a non minified part.

Screenshot 2023-01-04 at 2 21 03 pm
CGLOB commented 1 year ago

I have solve under jetbrains://php-storm/navigate/reference?project=magento2ce&path=~/Projects/JShrink/src/JShrink/Minifier.php file. but not able to create PR due permission issue. With my solution test passed as well attached the screenshot. [

Screen Shot 2023-03-02 at 6 50 21 pm

](url)

tedivm commented 1 year ago

What permission issue are you getting trying to make a PR? This repository is open for pull requests.

tedivm commented 1 year ago

Is this the same issue we discussed in the PRs? Is it okay to close it now?

CGLOB commented 1 year ago

Is this the same issue we discussed in the PRs? Is it okay to close it now?

yes it is same issue. I am verifying with new version. If all good i will inform you the same.

CGLOB commented 1 year ago

Hi Robert, It is reproducible in latest version. Anyway i will create new PR https://github.com/tedious/JShrink/pull/124 with same solution along with test coverage as you suggested earlier. "To do that put a file named "regex_end_string.js" to-

./JShrink/tree/master/tests/Resources/jshrink/input/

And then add a second file with the expected output to-

./JShrink/tree/master/tests/Resources/jshrink/output/ "

CGLOB commented 1 year ago

Hi @tedivm any update about the PR? with my solution js is minifying properly.

tedivm commented 1 year ago

@CGLOB your PR has failing tests at the moment. If you can get those resolved I can give it a proper review.

CGLOB commented 1 year ago

@CGLOB your PR has failing tests at the moment. If you can get those resolved I can give it a proper review.

I need your help. regex_close.js output is not correct. The output need to be modified. My solution is related regex_close.js code which suppose to minify fully. It should be "function test(string){return(string||'').replace(/([\!"#$%&'()*+,./:;<=>?@[]^`{|}~])/g,'\$1')}". Your latest test code conflicting the solution. Your help will be much appreciated. and about Libraries:jquery_ui.js i am checking.

CGLOB commented 1 year ago

@tedivm I have fixed the issues and test passed as well. Can you please verify once.

CGLOB commented 1 year ago

@tedivm any update about PR https://github.com/tedious/JShrink/pull/124 review?

tedivm commented 1 year ago

I'm brainstorming some adversarial strings- basically I'm just trying to beat this up some more to make sure there are no regressions. Regex is one of the most annoying parts of processing, and I've made changes in the past that I thought were great that added in regressions. As a result I'm super paranoid about changes to that part of the code base.

CGLOB commented 1 year ago

I'm brainstorming some adversarial strings- basically I'm just trying to beat this up some more to make sure there are no regressions. Regex is one of the most annoying parts of processing, and I've made changes in the past that I thought were great that added in regressions. As a result I'm super paranoid about changes to that part of the code base.

Thanks a lot for the review and your input. Please let me know if i can or i must help you to solve the issue.