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

Tech: migrate to es version of lodash (and shows dist size stats) #20910

Closed
wants to merge 40 commits into from

Conversation

ndelangen
Copy link
Member

No description provided.

code/stats.json Outdated Show resolved Hide resolved
code/stats.json Outdated Show resolved Hide resolved
code/stats.json Outdated Show resolved Hide resolved
@ndelangen ndelangen self-assigned this Feb 3, 2023
@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Feb 3, 2023
@ndelangen
Copy link
Member Author

OK this makes sense.. the reason tree-shaking barely seems to work is because of this:

import * as STORYBOOKCOMPONENTS from '@storybook/components';
import * as STORYBOOKCHANNELS from '@storybook/channels';
import * as STORYBOOKEVENTS from '@storybook/core-events';
import * as STORYBOOKROUTER from '@storybook/router';
import * as STORYBOOKTHEMING from '@storybook/theming';
import * as STORYBOOKMANAGERAPI from '@storybook/manager-api';
import * as STORYBOOKCLIENTLOGGER from '@storybook/client-logger';

In short:
We prebundle the whole package, to then expose it to addons globally.
But we have no way of knowing what the addon might actually use, so we have to load the entire module, everything it exports.

I tested this by adding:

// code/lib/manager-api/src/index.tsx
export const THIS_IS_A_TREESHAKING_TEST = true;

Which is then never ever used anywhere...

But it's still preserved in the manager prebundled code, because.. we load it as a import * as X..

So.. to get smaller prebundles, we need to limit our public API.

code/stats.json Outdated Show resolved Hide resolved
code/stats.json Outdated Show resolved Hide resolved
@ndelangen ndelangen requested a review from shilman February 3, 2023 15:22
@ndelangen ndelangen changed the title migrate to es version of lodash (and shows dist size stats) Tech: migrate to es version of lodash (and shows dist size stats) Feb 3, 2023
@ndelangen
Copy link
Member Author

This depends on this: ComponentDriven/csf#62 getting merged.

@@ -117,11 +120,24 @@ const run = async ({ cwd, flags }: { cwd: string; flags: string[] }) => {
platform: platform || 'browser',
external: externals,

esbuildPlugins: [
esbuildAliasPlugin({
lodash: 'lodash-es',
Copy link
Contributor

Choose a reason for hiding this comment

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

If we alias all occurrences of lodash to lodash-es, we have to also make sure, that lodash-es is installed for every package. Does the aliasing also encounter for lodash usage in node_modules? If so, how can we even be sure, that the right version of lodash-es is installed instead? Can you explain this part to me. I am a bit confused.

Copy link
Member Author

Choose a reason for hiding this comment

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

we have to also make sure, that lodash-es is installed for every package

yes

Does the aliasing also encounter for lodash usage in node_modules?

Sort of, when the bundler is following the modules-graph up, it will encounter imports to lodash and swap it out.
However when it encounters a module that's external, it will skip, and leave the import as-is.

This means if package-a depends on lodash, but package-a is externalized, it will NOT be swapped.

esbuildAliasPlugin({
lodash: 'lodash-es',
}),
lodashTransformer({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this plugin? Is tree-shaking not correctly working for lodash-es? Or do we want to accommodate all lodash usages, which we haven't swapped out by lodash-es?

Comment on lines +161 to +173
plugins: [
{
name: 'lodash',
async renderChunk(code) {
return { code: code.replaceAll('lodash-es', 'lodash') };
},
},
],
esbuildPlugins: [
esbuildAliasPlugin({
'lodash-es': require.resolve('lodash'),
}),
],
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this. Can you explain it to me?

Base automatically changed from norbert/stats-analysis-baseline to norbert/bench-dist-stats February 15, 2023 13:30
@ndelangen ndelangen closed this Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants