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

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

Open
CGLOB opened this issue Mar 2, 2023 · 12 comments
Open

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

CGLOB opened this issue Mar 2, 2023 · 12 comments

Comments

@CGLOB
Copy link

CGLOB commented Mar 2, 2023

[ 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
Copy link
Author

CGLOB commented Mar 2, 2023

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

@tedivm
Copy link
Member

tedivm commented Mar 2, 2023

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

@tedivm
Copy link
Member

tedivm commented Mar 5, 2023

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

@CGLOB
Copy link
Author

CGLOB commented Mar 6, 2023

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
Copy link
Author

CGLOB commented Mar 7, 2023

Hi Robert,
It is reproducible in latest version.
Anyway i will create new PR #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
Copy link
Author

CGLOB commented Mar 16, 2023

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

@tedivm
Copy link
Member

tedivm commented Mar 16, 2023

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

@CGLOB
Copy link
Author

CGLOB commented Mar 16, 2023

@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
Copy link
Author

CGLOB commented Mar 16, 2023

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

@CGLOB
Copy link
Author

CGLOB commented Mar 20, 2023

@tedivm any update about PR #124 review?

@tedivm
Copy link
Member

tedivm commented Mar 23, 2023

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
Copy link
Author

CGLOB commented Mar 23, 2023

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.

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

No branches or pull requests

2 participants