-
Notifications
You must be signed in to change notification settings - Fork 34
added support for Date #61
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
Conversation
… a Date, added simple check for Date class and ignored detected nodes
… (even if they have the same name)
| ts.createIdentifier('object'), | ||
| ts.createToken(ts.SyntaxKind.InstanceOfKeyword), | ||
| ts.createPropertyAccess( | ||
| ts.createIdentifier('global'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will work for NodeJS apps, but in the browser global should be window.
If you leave out global/window it'd work on both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't leave it out (to avoid conflicts with other classes of the same name) but I could add a check if global is available and test against window if it ins't. How about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose my proposal won't work because previously defined custom class Date will trigger a false positive.
I guess a polyfill will need to be added for globalThis which can then be used.
https://blog.logrocket.com/what-is-globalthis-why-use-it/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a check sounds good. How will you achieve it? If I try to reference global in the browser I get a ReferenceError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not yet tried i but my best guess at the moment would be: try to reference it, use it if it works or use window it that get's us the ReferenceError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found nicer a better way that doesn't require to trigger an error:
if (typeof global === 'undefined')
// use window
else
// use global|
I've changed the implementation of the check as mentioned above. |
|
Excellent! I will merge and release this. |
We needed support for Dates in interfaces so I've added it. This only targets the global Date class and does not add any support for any other classes.
I've made it so that the check still happens even if the "ignoreClasses" is set.
The relevant unit tests have been added/changed.