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.

Sign up for GitHub

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

Jump to bottom

feat(request-maker): use upstream httpsnippet #663

Merged
merged 7 commits into from
Nov 19, 2020
Merged

feat(request-maker): use upstream httpsnippet #663

merged 7 commits into from
Nov 19, 2020

Conversation

marcelltoth
Copy link
Contributor

Resolves #628

Depends on DefinitelyTyped/DefinitelyTyped#48987

After merging this PR we should archive stoplightio/httpsnippet

@marcelltoth marcelltoth requested a review from a team October 21, 2020 13:08
@marcelltoth marcelltoth self-assigned this Oct 21, 2020
@netlify
Copy link

netlify bot commented Oct 21, 2020 •
edited
Loading

Deploy preview for stoplight-elements ready!

Built with commit e3611f3

https://deploy-preview-663--stoplight-elements.netlify.app

@marcelltoth marcelltoth requested a review from a team October 28, 2020 12:59
mallachari
mallachari approved these changes Oct 28, 2020
View reviewed changes
Copy link

@mallachari mallachari left a comment

Choose a reason for hiding this comment

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

Works good!
There are TS problems regarding importing modules. Check out test step.

@marcelltoth
Copy link
Contributor Author

There are TS problems regarding importing modules. Check out test step.

I'm aware. There are some DefinitelyTyped issues: DefinitelyTyped/DefinitelyTyped#49170

Also we make quite a lot of lies in the toHAR method, it returns something that barely resembles a HAR object. Will fix that right after my DT HAR PR gets merged.

marcelltoth
marcelltoth commented Oct 30, 2020
View reviewed changes
packages/elements/src/stores/request-maker/request.ts
params: getEnabledParams(this.formDataParams).map(p => ({
name: p.name,
fileName: p.name,
value: p.value,
value: p.value instanceof File ? '<file contents>' : p.value,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are intentionally dummy. I wanted to make the least intrusive change possible while correcting the type violations.

@marcelltoth marcelltoth requested review from a team and mallachari October 30, 2020 16:26
mallachari
mallachari approved these changes Oct 30, 2020
View reviewed changes
Copy link

@mallachari mallachari left a comment

Choose a reason for hiding this comment

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

Still good to me 👍
There's some build error but it looks irrelevant. Locally works fine.

@mallachari
Copy link

Oh, the tests are failing. I think you forgot to add new snippet data

@marcelltoth
Copy link
Contributor Author

Tests fixed.

Unfortunately it looks like the build cannot be fixed just yet.
Kong did not publish a release that contains the fix (Kong/httpsnippet@0fc7f4a) yet. This is the master commit, was committed on Oct 6 but the latest release is September 25.

Iceboxing this until the upstream issue is resolved

@marcelltoth marcelltoth marked this pull request as draft November 2, 2020 08:03
@marcelltoth marcelltoth marked this pull request as ready for review November 19, 2020 13:24
@marcelltoth
Copy link
Contributor Author

I convinced Kong to release a new version 🎉 😇

So this is good to go. Rebased.

@marcelltoth marcelltoth merged commit 5db3159 into beta Nov 19, 2020
@marcelltoth marcelltoth deleted the feat/httpsnippet branch November 19, 2020 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mallachari mallachari mallachari approved these changes

@philsturgeon philsturgeon philsturgeon approved these changes

Assignees

@marcelltoth marcelltoth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop stoplightio/httpsnippet and use it's upstream instead
3 participants
@marcelltoth @mallachari @philsturgeon