-
Notifications
You must be signed in to change notification settings - Fork 141
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
Add new buffered methods, improve types #561
Conversation
🦋 Changeset detectedLatest commit: 822b057 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
1d0b9d6
to
63cda2b
Compare
b964248
to
8e8e76f
Compare
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.
Love seeing the addition of more methods on the buffered Analytics class! Had some pretty minor comments.
properties?: object & { | ||
[k: string]: JSONValue | ||
} | ||
properties?: EventProperties |
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 might be too strict - we have some users passing in an instance of a class as event properties and TypeScript will complain that the class doesn't have the index signature.
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 don’t think the official typescript interface should support people passing “any class”, since it’s AFAIK not possible in TS to distinguish between an intentionally passed class instance and an accidentally-passed function or array — the problem with the “object” type is that so much in JS is an object.
I think we should write the typescript API to encourage people to do things idiomatically, i.e create a toJSON() on their class or construct an object literal using class properties via {…new MyClass()} — but accepting any class seems like smell to me — and, as a consumer, the looseness of object would be a source of confusion (and uncaught typescript bugs).
Since this isn’t a runtime change, and is a non breaking change (in the Typescript “correctness” sense) that affects a super minority of npm typescript users, I feel good about this. They are always free to keep using vanilla JS and ignore or unsafely cast in their own calling code, but I want us to lead them into the light — and also do a better job of using Typescript to clarify and document the API (e.g using overloads).
/my two cents
8e8e76f
to
d27f0f5
Compare
This PR:
😢 Because of some differences in return types between AnalyticsBrowser and Analytics (AnalyticsBrowser always being async, while Analytics is inconsistent in that regard) the dream of a unified AnalyticsCore interface between Analytics and AnalyticsBrowser is on hold.
It’s IMO advisable to not try to obsess over strict sharing of a public interfaces, as that common abstraction does not seem possible, and it can in addition become hard to maintain either for the legacy behavior of one system or due to subtle differences between platforms. Ultimately, our API is just a bags of methods and we want to evolve and be nimble, and not hamstring ourselves.
We can continue to share models and types where appropriate.