Skip to content
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

feat(surveys): return API only surveys method #736

Closed
wants to merge 3 commits into from

Conversation

liyiy
Copy link
Contributor

@liyiy liyiy commented Jul 11, 2023

Changes

Add a new method to return API type surveys only. Update the existing getActiveMatchingSurveys to return non API surveys only.

To go in with: PostHog/posthog#16501

Checklist

@liyiy liyiy added the bump minor Bump minor version when this PR gets merged label Jul 11, 2023
@github-actions
Copy link

github-actions bot commented Jul 11, 2023

Size Change: +2.59 kB (0%)

Total Size: 668 kB

Filename Size Change
dist/array.full.js 173 kB +648 B (0%)
dist/array.js 114 kB +648 B (+1%)
dist/es.js 114 kB +648 B (+1%)
dist/module.js 114 kB +648 B (+1%)
ℹ️ View Unchanged
Filename Size
dist/recorder-v2.js 93.6 kB
dist/recorder.js 58.3 kB

compressed-size-action

@liyiy liyiy changed the title Return API only surveys feat(surveys): return API only surveys method Jul 11, 2023
@neilkakkar
Copy link
Collaborator

Hmm, this doesn't make sense to me - why are we excluding API survey type from getActiveMatchingSurveys ?

@neilkakkar
Copy link
Collaborator

Also, why is API a separate type? 🤔

@liyiy
Copy link
Contributor Author

liyiy commented Jul 11, 2023

Hmm, this doesn't make sense to me - why are we excluding API survey type from getActiveMatchingSurveys ?

Because we don't want to display API surveys in our use of the call 🤔

Basically users who are creating custom UI surveys only need API type surveys through the new method

@liyiy
Copy link
Contributor Author

liyiy commented Jul 11, 2023

Also, why is API a separate type? 🤔

I think Annika suggested it as a survey type which makes a lot of sense to me, where do you think storing that a survey is an API type belongs to instead?

@liyiy
Copy link
Contributor Author

liyiy commented Jul 11, 2023

This might make more sense as I push my PR up for the app API survey type @neilkakkar

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Commenting on the general approach

Why do we have separate endpoints? Why not just filter after the fact?

It's pretty straightforward for devs to do filtering after the fact so I would much prefer a simple and minimal API rather than custom methods for every sub-type of survey. We will end up in the future with dozens of top level methods that are all just doing some simple filtering.

posthog.getAllSurveys(surveys => {
  handleCustomSurveys(surveys.filter(s => s.type === "API"))
})

getActiveMatchingAPISurveys for example just sounds confusing. getAllSurveyMatches is even more confusing 😅

Being highly opinionated here as these choices we will have to live with and support for basically ever.

@liyiy
Copy link
Contributor Author

liyiy commented Jul 12, 2023

🙈 I was actually going to do that but then I thought once I did it you guys would tell me to add in separate methods for it in the library to make dev lives easier so they don't have to even filter for their survey types lol. If filtering is fine I'm going to close this PR then and just add it to the docs and surveys app instead

@liyiy liyiy closed this Jul 12, 2023
@liyiy liyiy deleted the return-api-surveys branch July 14, 2023 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants