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

Fixing EuiCodeBlock regex for Safari regex support. #6603

Merged
merged 7 commits into from
Feb 15, 2023
Merged

Fixing EuiCodeBlock regex for Safari regex support. #6603

merged 7 commits into from
Feb 15, 2023

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Feb 15, 2023

Summary

Yesterday I merged #6595 to update EuiCodeBlock regex to ignore template literal comments. This change caused a bug in Safari because it used the currently unsupported negative lookbehind. I've updated the regex to use negative lookahead and keyed off the end of the template literal string instead of the beginning.

Retested with Chrome, Safari, Edge, and Firefox locally as well as opened CodeSandbox from playground links in each browser. Expanded unit test for this case.

Changeset

  • Updated EuiCodeBlock regex to support Safari using negative lookahead.
  • Changed regex comment to explain negative lookahead.

Screenshot from Safari

safari-lookahead

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests

* Updated EuiCodeBlock regex to support Safari using negative lookahead.
* Changed regex comment to explain negative lookahead.
@1Copenut 1Copenut added bug documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog labels Feb 15, 2023
@1Copenut 1Copenut self-assigned this Feb 15, 2023
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6603/

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Unfortunately, this negative lookahead is not robust enough to catch the underlying issue of all import statements within template literals. It will not catch the following snippets:

const string = `import foo from 'bar';
const foo = bar;`

this will also not be caught:

const string = `/* this is js */
import foo from 'bar';
const bar = baz;`

Apologies on my end for not remembering Safari doesn't support negative lookbehinds (thanks for nothing, Apple) - we'll likely need to take a different approach to solving this problem (not catching strings within template literals) a different way than purely regex-based.

… case

- this prevents us from incorrectly catching usages just becase a backtick is on the same line as an import string, which it may not always be
- we mostly want to trim just line breaks caused by the removed imports - spaces shouldn't really be happening
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

@breehall do you mind reviewing this PR so I'm not self-approving/merging my own code?

@cee-chen
Copy link
Contributor

Going to add a changelog to this PR, since I intend on doing a patch release with this fix after this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6603/

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

This works for me. I tested by opening the PR preview link in Safari. The page loads with no trouble. Confirmed that the CodeSandbox demos containing imports loaded as expected without errors.

@cee-chen cee-chen merged commit abd38c6 into elastic:main Feb 15, 2023
@1Copenut 1Copenut deleted the bug/EuiCodeBlock-regex-safari branch July 25, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug documentation Issues or PRs that only affect documentation - will not need changelog entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants