Josee9988 / MinifyAll

A 𝗩𝗦𝗖𝗼𝗱𝗲 𝗺𝗢𝗻𝗢𝗳𝗢𝗲𝗿 for JS, JSON/C, CSS, and HTML, you will love its simplicity! 🌟 π˜Ύπ™€π™’π™₯π™§π™šπ™¨π™¨ and π™œπ™―π™žπ™₯ files and folders πŸ“¦ Reduce your bundle and file sizes with lightning speed ⚑
https://minifyall.jgracia.es/
GNU General Public License v3.0
71 stars 13 forks source link

Fixed false positive parsing #3

Closed pattishih closed 4 years ago

pattishih commented 4 years ago

Tbh, GitHub is still quite confusing to me. lol It looked like you had merged both of my submissions from my previous pull request, but the one that got pushed through to the VS Code update was my first commit rather than the 2nd one.

Now, this one is really important... My first commit in this new pull request has a critical error. I did not escape one of the . wildcards. The 2nd one needs to go through too if you were to merge the first one: https://github.com/Josee9988/MinifyAll/commit/25d5923324909b480fbec3b4c6b602c9baf831ca

Josee9988 commented 4 years ago

I saw that the '.' had something wrong but hadn't enough time to solve it, I'll merge this new branch as you say, thanks!

Josee9988 commented 4 years ago

Look, in the last .replace it says space is needed after a ','. So I'll simply let a space

pattishih commented 4 years ago

Oh, you must be using the "JS Standard" style? That's probably why it says you need a space.

Josee9988 commented 4 years ago

Seems like it is it really necessary, the space I mean, or can I left it without it?

pattishih commented 4 years ago

.replace(/\b0(\.\d+)/g, '$1');

The space between "/g," and "'$1'"? That should not be necessary, but you can add it.

Josee9988 commented 4 years ago

All right, many thanks @pattishih that means a lot for me!! Keep it up!

Josee9988 commented 4 years ago

@all-contributors please add @pattishih for infrastructure, maintenance

allcontributors[bot] commented 4 years ago

@Josee9988

I've put up a pull request to add @pattishih! :tada:

pattishih commented 4 years ago

Right now, the code is not robust to information specified in "contents:" and url sources (e.g., if a base64 code contains the characters "0px" just by random chance). The only way around this is to loop through the array and ignore these lines in the file. I've tried using regex lookbehind, which is available in VS Code because it uses Node.JS, but I've concluded that regex will not be enough for these situations.

Josee9988 commented 4 years ago

That "0px" was informed. I've looked into it months ago and the only solution is to look through the array and as you say ignore these lines, but it will make the extension slower and we can not just ignore a line. That will be left there until we find a better solution to that problem or just ignoring it. Maybe I'll just do what you say, but it will take some extra computational power.

I'll take a deep look in the upcoming week.

pattishih commented 4 years ago

I can help look into it. I have done a lot of text parsing in my past and present projects. I don't think it will be a problem to read it line by line, but maybe we can try timing it and see.

Josee9988 commented 4 years ago

Probs it is easier than I think, but I don't want to simply ignore a line because it has a 0px in a class name, because that line could contain some comments or things that need to be removed. Maybe an easy for look loop checking if that 0px is a class name and then forgetting about these characters.

Well... I think it can be done safely!:grinning:

Thanks for your effort!!

pattishih commented 4 years ago

oh, no, not ignore the entire line for everything! :) Just ignore specifics like class name (what you mentioned) and also the text inside "" of content and url() for src:... and for anything else you can think of?

Josee9988 commented 4 years ago

Class names, Id names, src, url, double quoted and single quoted strings and that might be all I do think:blush: