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
Fix broken terminal links with zero width spaces #995
Conversation
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Test suite run success881 tests passing in 455 suites. Report generated by 🧪jest coverage report action from bb4f83d |
This comment has been minimized.
This comment has been minimized.
return terminalLink(colors.green(stringifyMessage(this.value)), this.link ?? '') | ||
const text = colors.green(stringifyMessage(this.value)) | ||
const url = this.link ?? '' | ||
return terminalLink(text, url, {fallback: () => `${text} ( ${url} )`}) |
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.
I'd add a test that captures why the parenthesis is needed:
test("the link includes spaces between the URL and the parenthesis for command/control click to work", () => {
// ...
})
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.
you're right, added
Benchmark reportThe following table contains a summary of the startup time for all commands.
|
WHY are these changes introduced?
The
terminal-link
dependency adds a zero width space character to links.Why?
Because if you show something like
(https://shopify.com)
, the terminal will interpret that the last parenthesis is part of the URL likehttps://shopify.com)
and that will break it. So you need to add a space before the parenthesis for it to work, like:( https://shopify.com )
if you make that space then a zero-width space, it will look like there is nothing :)BUT
The terminal will assume the space is part of the URL and although most webs will ignore it, some won't (the link to the chrome store for the post purchase extension is broken because of this)
This is a known issue of
terminal-link
that was reported a year ago but hasn't been fixed yet (not sure there is a fix possible...)Fixes #991
WHAT is this pull request doing?
Add a normal space in the URLs instead of a zero-width one
BEFORE:
AFTER:
How to test your changes?
dev
and links to the post-purchase chrome extension should workMeasuring impact
How do we know this change was effective? Please choose one:
Checklist
dev
ordeploy
have been reflected in the internal flowchart.