Fixed a bug in EuiCodeBlock docs where import statement example was raised up and evaluated.#6595
Fixed a bug in EuiCodeBlock docs where import statement example was raised up and evaluated.#65951Copenut merged 4 commits intoelastic:mainfrom 1Copenut:bug/EuiCodeBlock-documentation
Conversation
…aised up and evaluated.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6595/ |
|
@1Copenut IMO we shouldn't change the example. But we should fix the regex that transforms the demo code to the If we pass a code like the following one: const jsCode = `/* I'm an example of JS */
import React from 'react';`;When we open the
The regex that generates the CC @cee-chen. |
Excellent call. I refactored the regex that populates the |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6595/ |
| // ([^]+?) - capture any characters (including newlines) | ||
| // from ('[A-Za-z0-9 -_.@/]*';) - ` from 'someLibrary';` - alphanumeric and certain special characters only | ||
| /(\/\/.+\n)?import ([^]+?) from ('[A-Za-z0-9 -_.@/]*';)/g, | ||
| /(\/\/.+\n)?import ((?!React)[^]+?) from ('[A-Za-z0-9 -_.@/]*';)/g, |
There was a problem hiding this comment.
While this works, I'm not a fan because it's not actually targeting the issue we want to solve - which is to say, it's finding an import string within a code snippet that isn't actually an import. So if the import snippet in question changed to import foo from 'bar', we'd still have the exact same issue.
I would strongly rather fix this with a negative lookbehind to find any backtick character before any import statement and not React specifically. You should be able to accomplish this like so:
| /(\/\/.+\n)?import ((?!React)[^]+?) from ('[A-Za-z0-9 -_.@/]*';)/g, | |
| /(?<!(`[\s\S]*))(\/\/.+\n)?import ([^]+?) from ('[A-Za-z0-9 -_.@/]*';)/g, |
Since regexes are incredibly hard to read, we should also document this new addition in the code above line 78:
// (?<!(`[\s\S]*)) - negative lookbehind ensuring that import statements aren't part of a template literal
// (\/\/.+\n)? - optional preceding comments that must be above specific imports, e.g. // @ts-ignoreThere was a problem hiding this comment.
I pulled in your suggested change and ran it through Regex101 with a few test strings. It works well for our purposes here, as long as the template literal is the last item we're looking for.
There was a problem hiding this comment.
There should be no scenario where a real import comes after a template literal backtick - our current eslint rules ensure that all legitimate imports are at the top of the file, so this is a fairly safe assertion to make.
|
Code changes LGTM - waiting for CI/staging to update and will approve then |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6595/ |
cee-chen
left a comment
There was a problem hiding this comment.
Staging LGTM! Nice attention to detail when catching this bug Trevor!
Summary
EUI end user reported a bug in the
EuiCodeBlocksandbox this weekend. After investigating I determined the sampleimport React from 'react'was being raised up and evaluated by CodeSandbox instead of being rendered as a code block. Ichanged the block to a general JS object and tested locally to ensure it rendered properly.adjusted the regex logic to ignore the string "React" in code comments.Before
After
QA
Remove or strikethrough items that do not apply to your PR.
General checklist