[steps] [Issue 2261] Fix multiline text messages #359
Conversation
Added test to check if the multiline message gets sent properly See: https://linear.app/expo/issue/ENG-11196/create-custom-build-example-slacking-team-members
Replacing all occurrences of `\\n` with `\n` after `JSON.stringify()` See: https://linear.app/expo/issue/ENG-11196/create-custom-build-example-slacking-team-members
Extracted the logic to a separate function and added tests for the case that came up during testing See: https://linear.app/expo/issue/ENG-11196/create-custom-build-example-slacking-team-members
|
Possibly, I can look into that, but I'm not sure at which step exactly the \n character gets doubly escaped. Since this fix doesn't touch any other functionality I thought we could ship it so the user can use the function as he wants. I'll check the interpolation again |
|
Besides, I think this issue happened not only on interpolated message contents, so I'm not sure if changing |
|
I was thinking that maybe the same problem happens in other places, not only when sending a Slack message. If it does, it'd be great to make the fix "global". |
|
I can agree with that, will look more into it |
Removed fixing escape characters from sendSlackMessage.ts to make it more global See: https://linear.app/expo/issue/ENG-11196/create-custom-build-example-slacking-team-members
Added escape character fix to value getter in BuildStepInput.ts See: expo/eas-cli#2261
Added tests for global fix for new line escape characters See: expo/eas-cli#2261
radoslawkrzemien
changed the title
|
@sjchmiela Made it global for all BuildStepInputs |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
==========================================
+ Coverage 91.45% 91.54% +0.09%
==========================================
Files 22 22
Lines 994 1004 +10
Branches 210 213 +3
==========================================
+ Hits 909 919 +10
Misses 85 85 ☔ View full report in Codecov by Sentry. |
| input: BuildStepInputValueTypeWithRequired<T, R> | ||
| ): BuildStepInputValueTypeWithRequired<T, R> { | ||
| if (typeof input === 'string') { | ||
| return input.replace(/\\n/g, '\n') as BuildStepInputValueTypeWithRequired<T, R>; |
There was a problem hiding this comment.
What about \\t? Is there a JS function, something more standard that unescapes this sort of thing?
There was a problem hiding this comment.
Not that I know of
There was a problem hiding this comment.
There is decodeURI[Component] which could decode these. However, a bigger question is why do these strings contain \\n and not \n? Maybe we don't need to decode input, but instead we should fix output?
There was a problem hiding this comment.
I agree, but atm I don't know if the problem is the earlier step setting the output, reading the outputs value, interpolating the input with other steps output, reading the interpolated input, or any other place.
I propose releasing this, since it actually fixes the problem that the user has and then digging deeper to look for the root cause
radoslawkrzemien
deleted the
@radoslawkrzemien/issue-2261-fix-multiline-text-messages
branch
Why
expo/eas-cli#2261
New line characters for messages dynamically generated in previous steps were incorrectly escaped, which resulted in incorrect formatting of the message received over Slack
How
Added a function that fixes incorrect escape characters when getting values from all inputs
Test Plan
Automated tests pass
Added more tests to cover multiline input as well as incorrectly escaped multiline input based on manual tests
Tested manually