-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
Typings for TypeScript #11
Comments
Hi @waterplea, I'm have never used TypeScript, so I would need some time to familiarize myself with it before I feel comfortable adding TypeScript-related code to the project. Unfortunately, extra time isn't something I don't have a lot of these days, so I can't guarantee if/when I will be able to get around to it. Two quick questions:
Thanks for taking the time to share the TypeScript definition and create the issue. Very much appreciated! |
Well, TypeScript basically just has types for variables so in order for it to know how to work with your library, you would need to tell it what are they. That's what I did with my snipped above. If your library only exports this As for your questions:
So yes, that's "nice to have". Problem is, I'm working on a library which means that every TypeScript project that would use my library would have to also add that file, unless I figure out how to include it with the library or you add it 😉 |
Hi again, @waterplea. Haven't forgotten about you. I've left this issue open as a reminder to investigate TypeScript to the point where I feel comfortable checking in the I hope you've managed to move forward while waiting for me. If not having the |
Typings are absolutely not necessary to use it in Typescript unless you set your compile to not allow these things (just to clear this up). Also, there's the DefinitelyTyped projects providing typings for libraries externally (but admittedly having it baked into the library is preferable). |
Two quick updates... The 1.8.0 release adds a new Also, I've decided against adding the typescript definitions to the project. Apologies to @waterplea, but the addition of the TypeScript is on my list of things to check out, but as I mentioned earlier I'm not sure when (or if) this will happen. If I do find the time, I will happily add a TypeScript file to the ponyfill once I feel comfortable maintaining it. Until then, perhaps @Airblader's DefinitelyTyped suggestion is the best way to go. Thanks for the thoughtful discussion, fellas. Very much appreciated. |
That's cool, we've configured our library to include the typings so it's no bother for our users. Yes, you would need them updated every time open API change if you ever decide to add them. BTW, I've made my own MutationObserver approach to use your ponyfill with Angular, where styles always change. Is you "watch" option optimized? Does it ignore STYLE tags with no use or declarations of variables? Does it go over everything everytime new style is added, or does it somehow determines what has to be updated and what could be left untouched? What I did was I've added data- attributes to style tags where variables were declared so that I would always include those with ponyfill, then I would monitor new STYLEs content for indexOf('--') to know if there are variables involved. And I would not go over old styles if there were no declaration of variables in new STYLEs. Don't know if that's any helpful to you, just wanted to share my thoughts on the matter and thanks for considering my issue. Keep up the great work! |
Good to hear you've baked the typings into you library without too much trouble. As for the MutationObserver implementation, it has been optimized. You can review the code here: css-vars-ponyfill/src/index.js Lines 320 to 372 in 9cc9f4d
Here's a quick summary of the optimizations:
There are more optimizations that could be done, but these made for a good first pass. |
Typings would be really easy to add into this repository, you just need to add the following line into your For With that line added, TypeScript users should be able to see the typings ;) |
Thanks @dubzzz (and @waterplea for the initial I know close to zero about TypeScript, but based on what I've read would the following typings not be more accurate (specifically, the declare module 'css-vars-ponyfill' {
export default function cssVars(options?: {
include?: string;
exclude?: string;
fixNestedCalc?: boolean;
onlyLegacy?: boolean;
onlyVars?: boolean;
preserve?: boolean;
silent?: boolean;
updateDOM?: boolean;
updateURLs?: boolean;
variables?: {[key: string]: string};
watch?: boolean;
onBeforeSend?(xhr: XMLHttpRequest, node: HTMLLinkElement|HTMLStyleElement, url: string): void;
onSuccess?(cssText: string, node: HTMLLinkElement|HTMLStyleElement, url: string): void;
onError?(message: string, node: HTMLLinkElement|HTMLStyleElement, xhr: XMLHttpRequest, url: string): void;
onWarning?(message: string): void;
onComplete?(cssText: string, styleNode: HTMLStyleElement): void;
}): void;
} As for adding these to the project, I'm open to doing so if there is a relatively painless way I can test the type definitions before release. I understand that adding a file and an entry in Perhaps "testing your type definitions file" isn't something folks typically do, or less-then-perfect type definitions for third-party scripts happen all the time and devs know how to work around them. I dunno. I'm just not up to speed on TypeScript. What I can't do is copy-paste code into the repo that I'm unable to test or maintain. If the answer is "rewrite all of your tests in TypeScript", that's a larger block of work than just including a Happy to discuss further, and I'm open to any suggestions. I'd love to provide type definitions for TypeScript users, but that has to be a prioritized below allowing me to properly maintain the project. |
Node types are more accurate, you could make them like that. As for testing, I frankly wouldn't bother as types here are rather clear and simple. You're not declaring you own types or anything like that, you just use primitives and interfaces that cannot be anything other than what you wrote. |
@jhildenbiddle @waterplea Seeing that the |
I'm less opposed to adding a The definitions are pretty straightforward, and recent changes like the addition of If @waterplea, @terrymun, @Airblader or @dubzzz would test these definitions and confirm they work as expected, I will add them to the repo. declare module 'css-vars-ponyfill' {
export default function cssVars(options?: {
rootElement?: HTMLElement|Node;
include?: string;
exclude?: string;
fixNestedCalc?: boolean;
onlyLegacy?: boolean;
onlyVars?: boolean;
preserve?: boolean;
shadowDOM?: boolean;
silent?: boolean;
updateDOM?: boolean;
updateURLs?: boolean;
variables?: {[key: string]: string};
watch?: boolean;
onBeforeSend?(xhr: XMLHttpRequest, node: HTMLLinkElement|HTMLStyleElement, url: string): void;
onSuccess?(cssText: string, node: HTMLLinkElement|HTMLStyleElement, url: string): void;
onError?(message: string, node: HTMLLinkElement|HTMLStyleElement, xhr: XMLHttpRequest, url: string): void;
onWarning?(message: string): void;
onComplete?(cssText: string, styleNode: HTMLStyleElement, cssVariables: {[key: string]: string}): void;
}): void;
} One additional question: What happens if devs use the ponyfill properly but the type definitions are incorrect (e.g. a the definitions specify a string instead of a boolean, or a callback definition is missing an argument)? I'm assuming the answer depends on how devs have TypeScript configured (display an error vs throw an error). Since I'm not using these definitions, my concern has always been that I would update the ponyfill, forget to make necessary changes to the |
@jhildenbiddle You have definitely raised some valid points: if any breaking changes are made to a plugin's interface, then the typings will have to be updated and it might break compatibility with older versions. I have actually used your definitions locally when importing In terms of deployment/hosting, it really depends on you: if you want it to be part of DefinitelyTyped or as a |
Regarding your question — TypeScript are just static checks, in the end it will be just JS so it really depends on your code. If definitions specify string instead of boolean, then developers will see that they have to pass string and it will most likely be typecasted by JS in your code so any string, other than empty will pass as true. Frankly, |
Now available in 1.12.0. Thanks for the discussion and help, fellas. |
Hey @jhildenbiddle, typings are there but are not automatically picked up. You need to add Please add it when you plan to update the library next time :) |
@waterplea -- Fixed in 1.16.0. Thanks for the heads up, btw. I told you I was a TS rookie. 😉 |
Hi John.
I have used your ponyfill with TypeScript and Angular. Works like charm although I had to add typing definitions to have it work. I know it is possible to include the definition file with the JS package somehow with package.json, would you consider adding it to the next release? Here's the file that I've made with all the proper types:
css-vars-ponyfill.d.ts
I'm not sure about how to add it so it would work, so I figured I could at least send you the file.
The text was updated successfully, but these errors were encountered: