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

More variables for better code compression #1489

Merged
merged 6 commits into from
Mar 10, 2019

Conversation

RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 19, 2018

This PR adds new temporary variables to some language definitions so the minifier can better compress the code.
Usually, this only saves about 100~200 bytes or so.
In the case of textile, we actually save about 0.6kb or about 16% of the minified file.

This also fixed the namespace pollution introduced by django.

This PR deals with syntactic changes only. If you spot a semantic change, please tell, that's a bug.

Fixed namespace pollution of django
Copy link
Member

@mAAdhaTTah mAAdhaTTah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change, but if it produces a size savings, it seems reasonable to me.

components/prism-parser.js Show resolved Hide resolved
@RunDevelopment
Copy link
Member Author

@mAAdhaTTah

Variables are now assigned on a separate line.

@mAAdhaTTah
Copy link
Member

@RunDevelopment I'm sorry, that's the opposite of what I intended. I think I confused you because I commented on a line that was correct, not one to fix. My apologies for that.

To be clear, I like assign the variable as well as to Prism.languages.language all on one line, the way you had it. When I saw it, I commented, intending to say "let's do it this way", fixing the ones that were on two (specifically asciidoc, and if there were others that I hadn't gotten to yet).

Sorry about that!

Copy link
Member

@mAAdhaTTah mAAdhaTTah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other question.

components/prism-django.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member Author

@mAAdhaTTah
Hahaha. Happens to the best of us.

@RunDevelopment
Copy link
Member Author

@mAAdhaTTah
Ok done.

Unrelated: Maybe we should expand the tests for django. I just made a mistake messing up the language definition a little. It wasn't detected by the tests...

@mAAdhaTTah
Copy link
Member

Unrelated: Maybe we should expand the tests for django. I just made a mistake messing up the language definition a little. It wasn't detected by the tests...

This would be great. You should be able to add additional tests in a separate PR without conflicting with this one.

@mAAdhaTTah
Copy link
Member

This looks good to me. A little concerned about introducing a lot of conflicts merging this. There's a PR for plsql I think that will be rough to land after this.

@RunDevelopment
Copy link
Member Author

That's right, we should land #1501 before this.

@RunDevelopment
Copy link
Member Author

Using a little copy function, I compressed AsciiDoc even further. This should also be a plus in terms of maintainability.

@mAAdhaTTah
Copy link
Member

@RunDevelopment If we resolve these conflicts, I'll give this a final review and get this in.

@RunDevelopment
Copy link
Member Author

Done.
Django is now left unchanged.

@RunDevelopment RunDevelopment merged commit bc53e09 into PrismJS:master Mar 10, 2019
@RunDevelopment RunDevelopment deleted the textile branch March 10, 2019 23:20
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.

2 participants