jhildenbiddle / css-vars-ponyfill

Client-side support for CSS custom properties (aka "CSS variables") in legacy and modern browsers
https://jhildenbiddle.github.io/css-vars-ponyfill
MIT License
1.46k stars 64 forks source link

Add interface export for options typings #126

Closed madeleineostoja closed 4 years ago

madeleineostoja commented 4 years ago

Just a small change, to export the interface for css-vars-ponyfill's options rather than leaving them anonymous in the declared module. Handy if you're ever extending css-vars-ponyfill or using it within another function that can pass on config (example)

codecov-io commented 4 years ago

Codecov Report

Merging #126 into master will increase coverage by 2.36%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   94.87%   97.23%   +2.36%     
==========================================
  Files           6        6              
  Lines         507      507              
==========================================
+ Hits          481      493      +12     
+ Misses         26       14      -12     
Impacted Files Coverage Δ
src/index.js 94.82% <0.00%> (+4.78%) :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 b62260f...a85116a. Read the comment docs.

jhildenbiddle commented 4 years ago

Thanks for the PR, @seaneking.

The one question I have relates to the interface name. I should state up front that I'm not a TypeScript guy (I know, I should be) so apologies if this is a silly concern.

Based on what I've read here the name CSSVarsPonyfillOptions seems okay, but is appending Options to the end of the name a standard or necessary? From my limited reading it seems like naming the interface CSSVarsPonyfill would suffice, but I look at that think some may confuse it with a class if it pulls up via Intellisense or something.

Thoughts?

madeleineostoja commented 4 years ago

Yep you can certainly name it just CSSVarsPonyfill, it doesn’t really matter. I was just being more descriptive (the interface describes the options object given to css-vars-ponyfill, not the function signature/module itself) but it’s all a wash. If CSSVarsPonyfill was a class-like module (eg a react component or something) then yeah there’d be a danger you’d be confusing users, but in this case I can’t think of any interface the module would export other than the options object, so it should be pretty self evident (if all else fails VS Code at least gives you a peak at the interface when importing it).

Happy to update the PR if you’d like?

jhildenbiddle commented 4 years ago

Nah. Let's keep it as is.

I noticed in the link you provided that you chose to rename the default cssVars export to cssVarsPonyfill. If other people are doing this, then it might be odd to have to deal with similarly named cssVarsPonyfill and CSSVarsPonyfill instanced.

As you suggested, appending Options to the end of the interface name is certainly more descriptive. Let's go with it.

jhildenbiddle commented 4 years ago

Sorry for the delay, @seaneking. I wanted to address #129 and this PR in the same release.

Thanks again for the contribution. Much appreciated!