jorgebucaran / classcat

Build a class attribute string quickly
MIT License
905 stars 22 forks source link

Adds conditional to throw error if selector would start with a number #9

Closed kyleshevlin closed 6 years ago

kyleshevlin commented 6 years ago

This PR fixes issue #8.

Since you're using wrap recursively, we can throw an error when type is a Number and no prefix is defined.

codecov-io commented 6 years ago

Codecov Report

Merging #9 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #9   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          13     15    +2     
  Branches        6      7    +1     
=====================================
+ Hits           13     15    +2
Impacted Files Coverage Ξ”
src/index.js 100% <100%> (ΓΈ) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update ab2e870...9ecdde0. Read the comment docs.

jorgebucaran commented 6 years ago

Hi @kyleshevlin thanks! πŸŽ‰

I will need adjust things a bit before I re-publish. I am sure we can skip the type === "number" if were are throwing. Also, I need to decide whether throwing at all is suitable for the library. I'll see if it's possible to make a build without the throw that only skips numbers or perhaps bails out early.

kyleshevlin commented 6 years ago

Yes, you could handle this with a /([0-9])^/.test(classes) instead of type and opt out entirely if that's what you wanted instead.

jorgebucaran commented 6 years ago

@kyleshevlin Yes, that's a good idea. I'll try out different combinations keeping in mind the benchmarks. I don't want to introduce a performance regression. πŸ˜„

jorgebucaran commented 6 years ago

@kyleshevlin Hi again! πŸ‘‹

Sorry, I had a change of heart and decided against this functionality, as I couldn't reconcile bundle size / performance with the usefulness of the feature. For example, you had to add code to check whether a prefix was being used or not, because that case represents the exception to the rule.

Although not on this PR, I also don't want to convert the number to a string, since your VDOM is most likely going to turn numbers to strings anyway.

cw(1) // => 1

I also don't think classwrap should throw and would prefer to pass on that responsibility to the user or the VDOM.