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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da0d3f7
Fixed a bug in EuiCodeBlock docs where import statement example was r…
1Copenut Feb 13, 2023
9de65d0
Refactored regex to ignore second React import group.
1Copenut Feb 13, 2023
5646c97
Added unit test for React import string in code blocks.
1Copenut Feb 13, 2023
30c7266
Refactored regex to be more fault tolerant. Renamed unit test.
1Copenut Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src-docs/src/components/guide_section/_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export const renderJsSourceCode = (code) => {
* Extract React import (to ensure it's always at the top)
*/
let reactImport = '';

renderedCode = renderedCode.replace(
// import - import + space
// (React)? - optional import `React` prefix - some files (like hooks) do not need it
Expand Down Expand Up @@ -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 •
edited
Loading

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.

(match) => {
remainingImports.push(match);
return '';
Expand Down
25 changes: 25 additions & 0 deletions src-docs/src/components/guide_section/_utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,5 +314,30 @@ describe('renderJsSourceCode', () => {
export default () => 'Hello world!';`)
);
});

it('handles importing React correctly by ignoring code comments', () => {
expect(
renderJsSourceCode({
default: dedent(`
import React from 'react';

import { v4 } from '@uuid/v4';

const jsCode = \`/* I'm an example of JS */
import React from 'react';\`;

export default () => 'Hello world!';`),
})
).toEqual(
dedent(`
import React from 'react';
import { v4 } from '@uuid/v4';

const jsCode = \`/* I'm an example of JS */
import React from 'react';\`;

export default () => 'Hello world!';`)
);
});
});
});