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: Show proper error when environment variables are not valued [INS-3641] #7227

Merged
merged 12 commits into from Apr 10, 2024

Conversation

CurryYangxx
Copy link
Member

@CurryYangxx CurryYangxx commented Apr 2, 2024

Related To INS-3641

This change is to give appropriate prompts when environment variables are missing, rather than directly displaying error messages.

Changes:

  • After clicking the send button, if the environment variable is missing, a modal(include the missing field) will show and let the user choose whether to continue the request or cancel.
  • If user choose continue,we need to ignore environment missing error in the request logic.
image

@CurryYangxx CurryYangxx requested a review from a team April 2, 2024 02:15
@CurryYangxx CurryYangxx requested review from a team and filfreire April 2, 2024 08:51
packages/insomnia/src/ui/components/request-url-bar.tsx Outdated Show resolved Hide resolved
packages/insomnia/src/ui/routes/request.tsx Outdated Show resolved Hide resolved
packages/insomnia/src/common/render.ts Outdated Show resolved Hide resolved
@jackkav
Copy link
Contributor

jackkav commented Apr 3, 2024

I'd suggest extracting the common modal to a different PR, its unclear from the description why its required in this scope.
also ignoreOnUndefined and throwOnUndefined are conflicting, would be better to use throwOnUndefined, since it is my understanding that we would now like to be able to send request with missing variables. Can explain more on slack.

@subnetmarco
Copy link
Member

"Execute anyways" (first letter upper case).

@CurryYangxx
Copy link
Member Author

CurryYangxx commented Apr 8, 2024

I'd suggest extracting the common modal to a different PR, its unclear from the description why its required in this scope. also ignoreOnUndefined and throwOnUndefined are conflicting, would be better to use throwOnUndefined, since it is my understanding that we would now like to be able to send request with missing variables. Can explain more on slack.

@jackkav The common modal code has been extracted to #7250

Copy link
Contributor

@jackkav jackkav left a comment

Choose a reason for hiding this comment

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

looks good so far, note: nunjucks will remove the template when ignoring it. This is not easy to change, we would need to replace nunjucks, or send unrendered text rather than a partial render with the template removed.

packages/insomnia/src/ui/components/request-url-bar.tsx Outdated Show resolved Hide resolved
@CurryYangxx
Copy link
Member Author

CurryYangxx commented Apr 8, 2024

This PR depends on #7250 ,please not merge until #7250 merged

@CurryYangxx
Copy link
Member Author

CurryYangxx commented Apr 9, 2024

looks good so far, note: nunjucks will remove the template when ignoring it. This is not easy to change, we would need to replace nunjucks, or send unrendered text rather than a partial render with the template removed.

@jackkav It is not easy to change.Now when user click the continue button, we will set the nunjucks throwOnUndefined config as true, so we can't perceived error and don't know if we need to use unrendered text.
There are two ways to solve.

  1. Keep the throwOnUndefined config as true, we handle error by ourselves.When there is a error, we use the origin text.But there is a problem, if user use multiple vars in one line, the result returned by the following line of code does not meet our expectations

input: ( '{{ var1 }}{{ var2 }}', { var1: 'aa' } ) output: '{{ var1 }}{{ var2 }}'

  1. replace nunjucks

I think that finding another template engine to replace Nunjucks would be better if necessary.

@CurryYangxx CurryYangxx requested a review from jackkav April 9, 2024 07:30
@jackkav
Copy link
Contributor

jackkav commented Apr 9, 2024

Understood perhaps we can reconsider point 1 and 2 in the future. To be clear our option 1 trade off is as follows.
When missing var2, we will show an error but if continue is pressed we do either:
Partial render and exclusion
input: ( '{{ var1 }}{{ var2 }}', { var1: 'aa' } ) output: 'aa'
Unrendered
input: ( '{{ var1 }}{{ var2 }}', { var1: 'aa' } ) output: '{{ var1 }}{{ var2 }}'

I find either satisfactory for this PR scope however for users coming from postman, I think Unrendered would be less surprising. I'll leave it to your judgement whether to make this change in a future PR.

jackkav
jackkav previously approved these changes Apr 9, 2024
@CurryYangxx CurryYangxx requested a review from jackkav April 9, 2024 11:28
@CurryYangxx CurryYangxx enabled auto-merge (squash) April 10, 2024 02:09
@CurryYangxx CurryYangxx merged commit 75c6a94 into develop Apr 10, 2024
7 checks passed
@CurryYangxx CurryYangxx deleted the feat/env-variables-proper-error branch April 10, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants