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

Fixed a bug in EuiCodeBlock docs where import statement example was raised up and evaluated. #6595

Merged
merged 4 commits into from
Feb 13, 2023
Merged

Fixed a bug in EuiCodeBlock docs where import statement example was raised up and evaluated. #6595

merged 4 commits into from
Feb 13, 2023

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Feb 13, 2023

Summary

EUI end user reported a bug in the EuiCodeBlock sandbox this weekend. After investigating I determined the sample import React from 'react' was being raised up and evaluated by CodeSandbox instead of being rendered as a code block. I changed 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

error


After

react

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
  • Added or updated jest and cypress tests
  • Checked Code Sandbox works for any docs examples
  • Checked for breaking changes and labeled appropriately

@1Copenut 1Copenut added bug documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog labels Feb 13, 2023
@1Copenut 1Copenut self-assigned this Feb 13, 2023
@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor

@1Copenut IMO we shouldn't change the example. But we should fix the regex that transforms the demo code to the demoJS tab.

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 DemoJS tab, the regex thinks that import React from 'react'; that was passed as a string and assigned to const jsCode is a real import. This is not correct. As we can see in the following screenshot the const jsCode is not correct. It doesn't contain the import that was moved to the third line. Consequently, the CodeSandbox fails.

Screenshot 2023-02-13 at 15 58 37

The regex that generates the DemoJS tab can be found here: https://github.com/elastic/eui/blob/main/src-docs/src/components/guide_section/_utils.js. Not sure if it is a lot of work to fix this... 😬

CC @cee-chen.

@1Copenut
Copy link
Contributor Author

IMO we shouldn't change the example. But we should fix the regex that transforms the demo code to the demoJS tab.

Excellent call. I refactored the regex that populates the remainingImports[] to ignore matches with "React" string in them. Added a unit test to confirm this behavior.

@kibanamachine
Copy link

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

@@ -80,7 +79,7 @@ export const renderJsSourceCode = (code) => {
// import - import + whitespace
// ([^]+?) - 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,
Copy link
Contributor

@cee-chen cee-chen Feb 13, 2023

Choose a reason for hiding this comment

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

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:

Suggested change
/(\/\/.+\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-ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

regextest

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@1Copenut 1Copenut requested a review from cee-chen February 13, 2023 21:29
@cee-chen
Copy link
Contributor

Code changes LGTM - waiting for CI/staging to update and will approve then

@kibanamachine
Copy link

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

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.

Staging LGTM! Nice attention to detail when catching this bug Trevor!

@1Copenut 1Copenut enabled auto-merge (squash) February 13, 2023 22:04
@1Copenut 1Copenut merged commit 8480ff7 into elastic:main Feb 13, 2023
@1Copenut 1Copenut deleted the bug/EuiCodeBlock-documentation 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 skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants