Skip to content
This repository was archived by the owner on Sep 23, 2021. It is now read-only.

Condense file sizes - #5

Open
gordoncl wants to merge 5 commits into
jeremys:masterfrom
gordoncl:convert-to-regexp
Open

Condense file sizes#5
gordoncl wants to merge 5 commits into
jeremys:masterfrom
gordoncl:convert-to-regexp

Conversation

@gordoncl

Copy link
Copy Markdown

I have a javascript application which slugifies titles on the client side. I wanted to use this library, but found that when it was uglied, it was rather large.

I've made my own smaller library, but am putting up this Pull Request, in case you want to incorporate the changes into your own library.

The new uglified L file is 4.9K, as opposed to the previous one, which was 102K.

I want to note two things:

  1. This uses the XML unicode database, which is all inclusive. The text format is separated into different files, which is why Hangul has special exceptions in the code. You might be able to remove the hangul and CJK conditions now that this uses the all inclusive database.

  2. I removed all characters outside of the BMP. I found that uslug does not support characters in the supplementary planes. As a lot of javascript engines (all as far as I know), treat surrogate pairs as two separate characters.

I would like to add support for this, but it would require a more complicated script to generate the regexp. If interested, let me know.

Comment thread lib/L.js Outdated
*
* 2) Extract relevant data.
*
* grep -E '<char cp="[0-9A-F]{4}"' ucd.all.flat.xml.pretty | grep -E 'gc="L[ultmo]"' | perl -n -e '/cp="([0-9A-F]{4})/ && print "$1\n"' | scripts/convert-to-regexp.sh

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I didn't know where to put the instructions. But these are the steps I took to compile the regular expression.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We could create a section in the README?

# Get rid of the initial separator
REGEXP="${REGEXP:1}"

echo "/$REGEXP/"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This script takes a stream of [0-9A-F]{4} strings and converts them into a single regular expression. So it can be piped with the commands listed in L. I'm not great at writing bash scripts and suspect that someone else might be able to provide a better implementation.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not much better at this myself. If it does the job, it's fine by me.

@jeremys

jeremys commented Feb 4, 2017

Copy link
Copy Markdown
Owner

Thanks for the PR! I'm not using uslug much now so I don't have strong opinions, your PR looks good to me. I'll probably just approve your changes once I have some free time. Some questions below:

  • Could you update the README to mention instructions about updating unicode lists? And the 2 notes you posted in your PR?
  • Is there some test cases we could add to further verify this or is the current set of tests enough?
  • Finally, would you mind providing me with a short release note that I can use when pushing this new version on NPM?

Thanks again!

Comment thread scripts/convert-to-regexp.sh Outdated
# After EOF, check updating the regexp one more time.
update_regexp

# Get rid of the initial separator

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Nit: add a period at the end

@gordoncl
gordoncl force-pushed the convert-to-regexp branch 4 times, most recently from 397a5a5 to ed34656 Compare February 12, 2017 21:20
@gordoncl

Copy link
Copy Markdown
Author

Hey @jeremys,

I moved the comments out of the lib files and put them into the README. Maybe you can take a look and see if what I've done makes sense.

I only added one test and that was to verify that a character that is marked as a letter in the supplementary plane (and in the original array) is, in fact, removed.

I think the current tests are adequate. Although, if we want to let someone know that the number of valid characters are changing, we could add a test which iterates through each character and verifies the number of valid characters is the same. For example, I could add something like this:

var EXPECTED_VALID_CHARACTERS = 49995;
var validCharacterCount = 0;

for (var charCode = 0; charCode < 0xFFFF; charCode++) {
  if (uslug(String.fromCharCode(charCode)).length === 1) {
    validCharacterCount++;
  }
}

validCharacterCount.should.equal(EXPECTED_VALID_CHARACTERS);

Let me know if you want this and I can put it in. The current set does support more characters, as the XML is more inclusive.

Hrm, not sure about the release note. The important piece of information to the end user seems to be that the size of the library has decreased. Maybe something simple, along the lines of:

Reduce size of library.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants