Skip to content

feat: support for making installation string generation dynamic - #269

Merged
erunion merged 4 commits into
mainfrom
feat/make-installation-dynamic
Apr 8, 2025
Merged

feat: support for making installation string generation dynamic#269
erunion merged 4 commits into
mainfrom
feat/make-installation-dynamic

Conversation

@erunion

Copy link
Copy Markdown
Member

🧰 Changes

We're working on some enhancements for HTTP snippet generation for external partners and need to make plugin installation strings dynamic -- this reworks how installation is set up on a client basis to allow for this.

@erunionerunion added the enhancement New feature or request label Apr 8, 2025
Comment threadsrc/targets/index.ts Outdated
@erunion
erunion marked this pull request as ready for review April 8, 2025 22:19
@erunion
erunion requested a review from kanadguptaApril 8, 2025 22:19
Comment threadsrc/targets/index.ts Outdated
Comment threadsrc/index.ts Outdated
Comment threadsrc/targets/javascript/axios/client.ts Outdated
Comment threadsrc/index.ts Outdated
}

const { installation } = target.clientsById[clientId || target.info.default];
const results = this.requests.map(request => (installation ? installation(request, options) : false));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i don't totally follow why this is an array. is it because you can pass several HAR files into this library?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah that's correct. it's not something we've ever done and is just holdoverr from upstream

@erunion
erunion requested a review from kanadguptaApril 8, 2025 22:53

@kanadguptakanadgupta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

looks great ty! reminder to publish this as a breaking change

expect(result).toBe('This was generated from a custom client.');

const install = snippet.installation('node', 'custom')[0];
expect(install).toBe('brew install https://httpbin.org/anything');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

hell yeah thank you!

Comment threadsrc/index.ts
return results;
}

installation(targetId: TargetId, clientId?: ClientId, options?: any) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

sorry one more thing — wanna make this return type a little stricter?

Suggested change
installation(targetId: TargetId,clientId?: ClientId,options?: any){
installation(targetId: TargetId,clientId?: ClientId,options?: any): (string|false)[]{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the inferred type is a little messy atm:

HTTPSnippet.installation(targetId: TargetId, clientId?: ClientId, options?: any): (string | false)[] | boolean[]

@erunionerunionApr 8, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

going to do isolateddeclarations in a followup

@erunion
erunion merged commit f5e3d50 into mainApr 8, 2025
@erunion
erunion deleted the feat/make-installation-dynamic branch April 8, 2025 23:38
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erunion@kanadgupta