Skip to content
This repository has been archived by the owner on Sep 3, 2022. It is now read-only.

[TypeScript] explicitly defines call patterns for page method #181

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dimitropoulos
Copy link

I noticed that the type for page previously required all arguments, but this doesn't match the behavior described in the tests or the library itself.

I made what is here by looking at the tests (and not the library source code) because I had quite a hard time thinking through all the combinations the library source code allows (and, after all, the tests already seem to have done that).

Copy link
Contributor

@bryanmikaelian bryanmikaelian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @dimitropoulos! Changes look fine but it appears your tests are failing. Here is the error message I am seeing when I run make test locally:

1) should accept (name)
     Analytics #page
     Can't find variable: test
/var/folders/tx/34knkw392dj3k7sbj709ggn00000gn/T/test/analytics.test.js:736:0 <- /var/folders/tx/34knkw392dj3k7sbj709ggn00000gn/T/c2c57f9139c4399452aacd0bc52983aa.browserify:35039:25

Once we fix that up, we can merge this!

@dimitropoulos
Copy link
Author

should be all set. I will note that calling with just name is, now, the only way to call page this without a callback. Is that correct or is the callback (as a last argument) always optional?

@bryanmikaelian
Copy link
Contributor

bryanmikaelian commented Aug 17, 2020

@dimitropoulos Ah, the callback is always optional. We'll definitely want the types to reflect that. From our docs

An optional function that is executed after a short timeout, giving the browser time to make outbound requests first.

This is true for any type of call (Track, Identity, etc)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants