Identifiers & unquoted property names

[jskrzypek]

@chriso Can you look over this and tell me what I should add/fix? Where should I add the tests?

[coveralls]

Coverage remained the same at 99.091% when pulling 2615860 on jskrzypek:feat-identifiers into ce31057 on chriso:master.

[chriso]

@jskrzypek some tests and a README entry would be great, thanks.

[jskrzypek]

@chriso Of course, happy to do both (I wouldn't expect the PR to be accepted without them!)

For the tests, should just add them as it(...)s to the big 'Validators' describe? Should I try to maintain the tests order in some way?

[chriso]

@jskrzypek great 😄 Adding tests to the end of that test suite will be fine.

[jskrzypek]

👍 I'll get on this today, just have to finish up some changes on one of our projects.

[jskrzypek]

Okay I wrote tests for everything I wrote (feel free to double check me) I'll ad a readme tomorrow

[coveralls]

Coverage decreased (-0.6%) to 98.505% when pulling 7148687 on jskrzypek:feat-identifiers into ce31057 on chriso:master.

[coveralls]

Coverage decreased (-0.002%) to 99.089% when pulling 23670cc on jskrzypek:feat-identifiers into ce31057 on chriso:master.

[coveralls]

Coverage increased (+0.08%) to 99.172% when pulling 7e8b087 on jskrzypek:feat-identifiers into ce31057 on chriso:master.

[coveralls]

Coverage increased (+0.08%) to 99.172% when pulling c75b6ec on jskrzypek:feat-identifiers into ce31057 on chriso:master.

[coveralls]

Coverage increased (+0.08%) to 99.172% when pulling c75b6ec on jskrzypek:feat-identifiers into ce31057 on chriso:master.

[chriso]

Thanks, I can't see any issues with the code after a quick pass.

I can't merge this in its currently state though unfortunately, as the client-side version of the library is now 3x larger (17.5 KB => 54.5 KB) and IMO it's already bloated enough as it is.

Something like #531 would help by allowing users to pick and choose the validators they need in their front-end build. I'll leave this PR open in the mean time.

[jskrzypek]

_Yikes!_ Wow, I did not look at the minified version until now. I'm actually kind of impressed with how unoptimized that is... 😆

Ok I'll see if I can can somehow replace the regex with something equivalent in how it evaluates unicode... hopefully that will be cleaner...

... the longest regex is 14654 characters 😮

[jskrzypek]

So after a bunch of research it looks like the only clean way to do this without obscenely long regexes will be to either wait until this tc39 proposal for unicode properties in RegExp lands (seems like it will, but not sure when) to do something like the following for the ES5/6 matching:

/^(?:\p{ID_Start}|\$|_|\\u[a-zA-Z0-9]{4}|\\u\{[a-zA-Z0-9]{1,}\})(?:\p{ID_Continue}|\$|_|\\u[a-zA-Z0-9]{4}|\\u\{[a-zA-Z0-9]{1,}\}|\u200C|\u200D)*/

All of the other solutions ultimately fall back on including the obscenely long regexes for backwards compatibility.

I also realized that estools/esutils already includes the equivalent of my isIdentifier validation script so people who want that functionality can use that library.

I think I'll do 2 things. First I'll break off the numeric property name validation part of isUnquotedPropertyName into it's own validator. If you want, I can re-submit that alone as a separate PR, since it's basically the only added functionality on top of the https://github.com/estools/esutils method, so people can use them together. In the meantime, I'll merge this PR to the master on my fork (jskrzypek/validator.js), so people (like me) who want the isIdentifier and isUnquotedPropertyName validators and don't mind the bulk of the bundle can have them in the same lib as the rest of validator.js. This could be temporary until something is landed for #531.

[chriso]

That tc39 proposal looks very useful, but it unfortunately won't reduce the size of the minified front-end version where we still need to support the lowest common denominator browsers.

I'd be happy to accept this PR once we have a build system in place like lodash, and happy to accept a smaller PR in the mean time. Thanks again 😄

[jskrzypek]

I think that for now the current numeric validators suffice for validating if property names are numeric. We can circle back to this more integrated solution when the build system lands. In the meantime I'm going to be using the esutils validator in my project.

[chriso]

Closing this out due to conflicts. We can revisit if/when custom builds are supported (#531).