-
Notifications
You must be signed in to change notification settings - Fork 126
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
[ShopifyVM] 'app logs' Show input query variables in output for FunctionRunLog #4566
base: main
Are you sure you want to change the base?
Conversation
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
app logs
command for function runs
Coverage report
Show files with reduced coverage 🔻
Test suite run success1911 tests passing in 867 suites. Report generated by 🧪jest coverage report action from b318ad7 |
7c60a20
to
685540c
Compare
We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset. |
d104c32
to
ef0df79
Compare
packages/app/src/cli/services/app-logs/logs-command/ui/components/Logs.test.tsx
Show resolved
Hide resolved
packages/app/src/cli/services/app-logs/logs-command/ui/components/Logs.test.tsx
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/app-logs/logs-command/ui/components/Logs.test.tsx
Show resolved
Hide resolved
<Box marginLeft={1} marginTop={1}> | ||
<Text> | ||
{prettyPrintJsonIfPossible(appLog.inputQueryVariablesMetafieldValue) || ( | ||
<Text color="red">No metafield data found!</Text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe another way of stating this is: Metafield is not set
@nickwesselman WDYT of this?
packages/app/src/cli/services/app-logs/logs-command/ui/components/Logs.tsx
Show resolved
Hide resolved
packages/app/src/cli/services/app-logs/logs-command/ui/components/Logs.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM- just the one issue with a lot of use of the word webhook
in the tests that I think isn't appropriate- network access relates to network access in functions (though given how often we talk about webhooks in this project and the naming here I can see where the confusion would come from)
packages/app/src/cli/services/app-logs/logs-command/ui/components/Logs.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't 🎩 but code LGTM!
47ba46e
to
86135c0
Compare
Quick video if anyone wants to take a look at changes. Will rebase PR before merging. |
Thanks for the video @mssalemi! Things look good in |
The variables get added to the payload in the logs:
perhaps we want to decode the the value 👀 |
86135c0
to
84e2ef1
Compare
Can we attempt to parse the value and output it as an object if successful? |
} else if (typeof json === 'object' && json !== null) { | ||
return JSON.stringify(json, null, 2) | ||
} else { | ||
if (typeof json === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the reasoning for this change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will look into this, its related to Nicks comment above. Right now with PR we dont decode that part of the payload, so to display the JSON string better I added that. But thinking, it probably makes more sense to decode the inner payload here. Will look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if you could add a return type so it's easier to grok what this function is trying to return!
My main question is why you removed the other cases from within the try/catch block instead of just changing the throw to return json.
Additionally, why are we changing this to return json instead of throwing an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sounds good, reverting this change. I changed up how I call this method and ensure the metafield is an object, that we can we leave this unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we changing this to return json instead of throwing an error?
Reverted this, but originally was because this values the we needed to use this method on is from a metafield, and it could technically just be a string, and we dont want to error in those cases and just show whatever the value was. That way partners can use it to debug any issue related to the input query variables.
Part of: https://github.com/Shopify/script-service/issues/6233
We're enhancing the debugging experience for developers by providing visibility into the input query variables used during a function run.
WHY are these changes introduced?
This PR will add an input query variables section that will conditionally show based on if input query variables are used. Input query variables will be used when the values are specified in the
toml
.WHAT is this pull request doing?
Add input query variables section to the
app logs
command:I also added some visual improvements to make the output and input sections more clear for partners to view. And updated the ouput and input sections to match.
Changes: with improved styling
With coloring (also will need to update other sections - alternate idea)
and when no metafield is found:
How to test your changes?
From local cli, to your spin instance:
** replace spin id and app path
Post-release steps
FunctionRunLog
andNetworkAccessResponseFromCacheLog
,NetworkAccessRequestExecutedLog
into separate files.Measuring impact
How do we know this change was effective? Please choose one:
Checklist