-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Logs] Amend lazy imports in logs_shared plugin #164102
Conversation
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
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.
This seems to fix it, thank you.
About making the dynamic
import type-safe, wouldn't narrowing down the LoadableComponent
type do the trick?
type LoadableComponent = () => Promise<{
default: React.ComponentType<any>;
}>;
Thanks for the review 🙏 Your suggestion for the stricter type also works, thanks. I thought I'd tried a similar variation yesterday, but apparently not 😅 I had to tweak one Hopefully the latest |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @Kerry350 |
Interesting how the async chunk size actually goes down without the page load bundle increasing 🤔 the mysteries of webpack... 🪄 |
## Summary As part of elastic#161151 a [selection of component imports were made lazy](https://github.com/elastic/kibana/blob/main/x-pack/plugins/logs_shared/public/index.ts#L52) and wrapped with a [`dynamic` wrapper component](https://github.com/elastic/kibana/blob/main/x-pack/plugins/logs_shared/common/dynamic.tsx#L22). Unfortunately some of these imports did not adhere to the rules of React's `lazy` imports (needing a `default` export, no named imports etc), and the `dynamic` wrapper seems to have suppressed error information that would have been available via using `lazy` directly. Only the anomaly and categories log entry examples (in the expanded rows) were affected by this, as the stream and embeddable import from locations that were backed by a `default` export (and those top level components don't import from that particular index file lower in the hierarchy). For imports that weren't backed by a `default` I've added them, and where necessary moved components to new files if needed (since it's one `default` per file). Also open to suggestions of ways we can alter the `<dynamic />` component and maintain the error safety 🤔 ## Examples Without these changes: ![Screenshot 2023-08-16 at 17 35 50](https://github.com/elastic/kibana/assets/471693/78aa0300-109e-40b5-b64f-6574a547cbf3) Warning using `lazy` directly without the `dynamic` wrapper: ![Screenshot 2023-08-16 at 17 36 27](https://github.com/elastic/kibana/assets/471693/a71e3c72-cf3a-4846-9ee9-df70c1729b03) ## Testing - Check all instances render correctly (stream, embeddable uses, and ML page log entry examples). (cherry picked from commit a96785c)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…4180) # Backport This will backport the following commits from `main` to `8.10`: - [[Logs] Amend lazy imports in logs_shared plugin (#164102)](#164102) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kerry Gallagher","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-17T13:44:57Z","message":"[Logs] Amend lazy imports in logs_shared plugin (#164102)\n\n## Summary\r\n\r\nAs part of #161151 a [selection of\r\ncomponent imports were made\r\nlazy](https://github.com/elastic/kibana/blob/main/x-pack/plugins/logs_shared/public/index.ts#L52)\r\nand wrapped with a [`dynamic` wrapper\r\ncomponent](https://github.com/elastic/kibana/blob/main/x-pack/plugins/logs_shared/common/dynamic.tsx#L22).\r\nUnfortunately some of these imports did not adhere to the rules of\r\nReact's `lazy` imports (needing a `default` export, no named imports\r\netc), and the `dynamic` wrapper seems to have suppressed error\r\ninformation that would have been available via using `lazy` directly.\r\n\r\nOnly the anomaly and categories log entry examples (in the expanded\r\nrows) were affected by this, as the stream and embeddable import from\r\nlocations that were backed by a `default` export (and those top level\r\ncomponents don't import from that particular index file lower in the\r\nhierarchy). For imports that weren't backed by a `default` I've added\r\nthem, and where necessary moved components to new files if needed (since\r\nit's one `default` per file).\r\n\r\nAlso open to suggestions of ways we can alter the `<dynamic />`\r\ncomponent and maintain the error safety 🤔\r\n\r\n## Examples\r\n\r\nWithout these changes:\r\n\r\n![Screenshot 2023-08-16 at 17 35\r\n50](https://github.com/elastic/kibana/assets/471693/78aa0300-109e-40b5-b64f-6574a547cbf3)\r\n\r\nWarning using `lazy` directly without the `dynamic` wrapper:\r\n\r\n![Screenshot 2023-08-16 at 17 36\r\n27](https://github.com/elastic/kibana/assets/471693/a71e3c72-cf3a-4846-9ee9-df70c1729b03)\r\n\r\n## Testing\r\n\r\n- Check all instances render correctly (stream, embeddable uses, and ML\r\npage log entry examples).","sha":"a96785cd2cf6dc28b6d786423e4604a7aee10c97","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Feature:Logs UI","Team:Infra Monitoring UI","v8.10.0","v8.11.0"],"number":164102,"url":"https://github.com/elastic/kibana/pull/164102","mergeCommit":{"message":"[Logs] Amend lazy imports in logs_shared plugin (#164102)\n\n## Summary\r\n\r\nAs part of #161151 a [selection of\r\ncomponent imports were made\r\nlazy](https://github.com/elastic/kibana/blob/main/x-pack/plugins/logs_shared/public/index.ts#L52)\r\nand wrapped with a [`dynamic` wrapper\r\ncomponent](https://github.com/elastic/kibana/blob/main/x-pack/plugins/logs_shared/common/dynamic.tsx#L22).\r\nUnfortunately some of these imports did not adhere to the rules of\r\nReact's `lazy` imports (needing a `default` export, no named imports\r\netc), and the `dynamic` wrapper seems to have suppressed error\r\ninformation that would have been available via using `lazy` directly.\r\n\r\nOnly the anomaly and categories log entry examples (in the expanded\r\nrows) were affected by this, as the stream and embeddable import from\r\nlocations that were backed by a `default` export (and those top level\r\ncomponents don't import from that particular index file lower in the\r\nhierarchy). For imports that weren't backed by a `default` I've added\r\nthem, and where necessary moved components to new files if needed (since\r\nit's one `default` per file).\r\n\r\nAlso open to suggestions of ways we can alter the `<dynamic />`\r\ncomponent and maintain the error safety 🤔\r\n\r\n## Examples\r\n\r\nWithout these changes:\r\n\r\n![Screenshot 2023-08-16 at 17 35\r\n50](https://github.com/elastic/kibana/assets/471693/78aa0300-109e-40b5-b64f-6574a547cbf3)\r\n\r\nWarning using `lazy` directly without the `dynamic` wrapper:\r\n\r\n![Screenshot 2023-08-16 at 17 36\r\n27](https://github.com/elastic/kibana/assets/471693/a71e3c72-cf3a-4846-9ee9-df70c1729b03)\r\n\r\n## Testing\r\n\r\n- Check all instances render correctly (stream, embeddable uses, and ML\r\npage log entry examples).","sha":"a96785cd2cf6dc28b6d786423e4604a7aee10c97"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/164102","number":164102,"mergeCommit":{"message":"[Logs] Amend lazy imports in logs_shared plugin (#164102)\n\n## Summary\r\n\r\nAs part of #161151 a [selection of\r\ncomponent imports were made\r\nlazy](https://github.com/elastic/kibana/blob/main/x-pack/plugins/logs_shared/public/index.ts#L52)\r\nand wrapped with a [`dynamic` wrapper\r\ncomponent](https://github.com/elastic/kibana/blob/main/x-pack/plugins/logs_shared/common/dynamic.tsx#L22).\r\nUnfortunately some of these imports did not adhere to the rules of\r\nReact's `lazy` imports (needing a `default` export, no named imports\r\netc), and the `dynamic` wrapper seems to have suppressed error\r\ninformation that would have been available via using `lazy` directly.\r\n\r\nOnly the anomaly and categories log entry examples (in the expanded\r\nrows) were affected by this, as the stream and embeddable import from\r\nlocations that were backed by a `default` export (and those top level\r\ncomponents don't import from that particular index file lower in the\r\nhierarchy). For imports that weren't backed by a `default` I've added\r\nthem, and where necessary moved components to new files if needed (since\r\nit's one `default` per file).\r\n\r\nAlso open to suggestions of ways we can alter the `<dynamic />`\r\ncomponent and maintain the error safety 🤔\r\n\r\n## Examples\r\n\r\nWithout these changes:\r\n\r\n![Screenshot 2023-08-16 at 17 35\r\n50](https://github.com/elastic/kibana/assets/471693/78aa0300-109e-40b5-b64f-6574a547cbf3)\r\n\r\nWarning using `lazy` directly without the `dynamic` wrapper:\r\n\r\n![Screenshot 2023-08-16 at 17 36\r\n27](https://github.com/elastic/kibana/assets/471693/a71e3c72-cf3a-4846-9ee9-df70c1729b03)\r\n\r\n## Testing\r\n\r\n- Check all instances render correctly (stream, embeddable uses, and ML\r\npage log entry examples).","sha":"a96785cd2cf6dc28b6d786423e4604a7aee10c97"}},{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Kerry Gallagher <[email protected]>
Summary
As part of #161151 a selection of component imports were made lazy and wrapped with a
dynamic
wrapper component. Unfortunately some of these imports did not adhere to the rules of React'slazy
imports (needing adefault
export, no named imports etc), and thedynamic
wrapper seems to have suppressed error information that would have been available via usinglazy
directly.Only the anomaly and categories log entry examples (in the expanded rows) were affected by this, as the stream and embeddable import from locations that were backed by a
default
export (and those top level components don't import from that particular index file lower in the hierarchy). For imports that weren't backed by adefault
I've added them, and where necessary moved components to new files if needed (since it's onedefault
per file).Also open to suggestions of ways we can alter the
<dynamic />
component and maintain the error safety 🤔Examples
Without these changes:
Warning using
lazy
directly without thedynamic
wrapper:Testing