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

feat: Lazy loading and code splitting #1802

Merged
merged 16 commits into from
Feb 26, 2024
Merged
2 changes: 1 addition & 1 deletion packages/chart/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export * from './ChartUtils';
export * from './DownsamplingError';
export { default as FigureChartModel } from './FigureChartModel';
export { default as MockChartModel } from './MockChartModel';
export { default as Plot } from './plotly/Plot';
export { default as Plot } from './plotly/LazyPlot';
bmingles marked this conversation as resolved.
Show resolved Hide resolved
export * from './ChartTheme';
export * from './ChartThemeProvider';
export { default as isFigureChartModel } from './isFigureChartModel';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { LoadingOverlay } from '@deephaven/components';
import { lazy, Suspense } from 'react';

const PlotBase = lazy(() => import('./PlotBase.js'));
const PlotBase = lazy(() => import('./Plot.js'));

function Plot(props: React.ComponentProps<typeof PlotBase>): JSX.Element {
return (
Expand Down
29 changes: 29 additions & 0 deletions packages/code-studio/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,35 @@ export default defineConfig(({ mode }) => {
emptyOutDir: true,
sourcemap: true,
target: 'esnext',
rollupOptions: {
output: {
manualChunks: id => {
/**
* Without this, our chunk order may cause a circular reference
* by putting the helpers in the vendor or plotly chunk
* This causes failures with loading the compiled version
*
* See https://github.com/rollup/plugins/issues/591
*/
if (id === '\0commonjsHelpers.js') {
return 'helpers';
}

if (id.includes('node_modules')) {
if (id.includes('monaco-editor')) {
return 'monaco';
}
if (id.includes('plotly.js')) {
return 'plotly';
}
if (id.includes('mathjax')) {
return 'mathjax';
}
return 'vendor';
}
},
},
},
},
optimizeDeps: {
esbuildOptions: {
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/theme/theme-spectrum/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import './theme-spectrum-overrides.css';
* e.g.
* {
* 'dh-spectrum-palette': '_dh-spectrum-palette_abr16_1',
* 'higher-palette-specificity': '_higher-palette-specificity_18mbe_1',
mattrunyon marked this conversation as resolved.
Show resolved Hide resolved
* 'dh-spectrum-alias': '_dh-spectrum-alias_18mbe_1',
* 'higher-alias-specificity': '_higher-alias-specificity_18mbe_1',
* }
*/
export const themeSpectrumClassesCommon = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/* stylelint-disable custom-property-empty-line-before */
/* stylelint-disable alpha-value-notation */
.dh-spectrum-alias {

/**
* Intentionally using the classname twice so we have higher specificity than spectrum's definitions
*/
Copy link
Member

Choose a reason for hiding this comment

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

Why are we doing this and why is it a part of this change? Should add to the comment why we need to do this (chunks loading out of order).

Though that being said - chunks loading out of order implies there's something off with the packaging. Why is this happening now?

Copy link
Collaborator Author

@mattrunyon mattrunyon Feb 20, 2024

Choose a reason for hiding this comment

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

I'll update the comment.

I didn't dig too far into why the chunks were out of order (we don't directly import spectrum's css nor can we because it's spread across every component). We could also have an import order wrong somewhere in our code. Or because of lazy loading the chunks are getting split a little differently. We still put node_modules into 1 of 4 chunks (monaco, plotly, mathjax, vendor). Everything else seems to chunk based on the dynamic imports.

Either way, just increasing the specificity is safer in the long run I think (although it did fail e2e tests because of the import order)

.dh-spectrum-alias.dh-spectrum-alias {
/*********** Override variables in spectrum-global.css **********************/
--spectrum-alias-background-color-default: var(--dh-color-bg);
--spectrum-alias-background-color-disabled: var(--dh-color-disabled-bg);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
.dh-spectrum-palette {
/**
* Intentionally using the classname twice so we have higher specificity than spectrum's definitions
*/
.dh-spectrum-palette.dh-spectrum-palette {
/* Gray */
--spectrum-gray-50: var(--dh-color-gray-50);
--spectrum-gray-75: var(--dh-color-gray-75);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
import {
AdvancedSettings,
IrisGrid,
IrisGridType,
type IrisGridType,
IrisGridModel,
IrisGridUtils,
isIrisGridTableModelTemplate,
Expand Down
2 changes: 1 addition & 1 deletion packages/iris-grid/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export * from './CommonTypes';
export { default as ColumnHeaderGroup } from './ColumnHeaderGroup';
export * from './PartitionedGridModel';
export * from './IrisGrid';
export { default as IrisGridType } from './IrisGrid';
export type { default as IrisGridType } from './IrisGrid';
export { default as SHORTCUTS } from './IrisGridShortcuts';
export { default as IrisGridModel } from './IrisGridModel';
export { default as IrisGridTableModel } from './IrisGridTableModel';
Expand Down
10 changes: 10 additions & 0 deletions tests/lazy-loading.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { test, expect } from '@playwright/test';

test('does not load heavy dependencies by default', async ({ page }) => {
mattrunyon marked this conversation as resolved.
Show resolved Hide resolved
page.on('request', request => {
expect(request.url()).not.toMatch('assets/plotly');
expect(request.url()).not.toMatch('assets/mathjax');
});
await page.goto('');
await expect(page.locator('.loading-spinner')).toHaveCount(0);
});
Loading