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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tree shaking #11164

Open
wants to merge 41 commits into
base: devel
Choose a base branch
from
Open

Tree shaking #11164

wants to merge 41 commits into from

Conversation

renanccastro
Copy link
Contributor

@renanccastro renanccastro commented Sep 8, 2020

Tree shaking! 馃槃

In this PR the focus will be on removing unused exports and code from the final bundle.

Description

Tree-shaking is a technic that the bundler uses to remove unused files and code from the final bundle. This PR implements 2 strategies:

  1. Remove unused Files, by checking es6 imports/exports
  2. Remove unused exports, by checking used imports.

This PR does not implement yet a verification for actual usage in the code, for tree shaking. It is restricted to imports, and also deals with dynamic and nested imports.

Usage

app

For enabling tree shaking in your app, simply add the flag:

sideEffects: false

to your package.json.

packages

For packages, you can use on package.js the following:

api.setSideEffects(false);

inside the Package.onUse section.

Checklist

  • Get a map of imports by static analyzing all isopacks
  • Convert relative import paths to absolute ones
  • Transform the resulting tree in the bundler to remove unused reify transformed exports: module.exports({ a => ...}).
  • Create a flag for packages.js to mark side effect free
  • Exclude packages with side effects from the elimination
  • Deal with const { ClientStream } = require("meteor/socket-stream-client");

Improvements

  • Improve import-scanner performance. Today, it's necessary to get rid of AST cache, as we need to check every possible usage for import/exports. One option is to use as the key, the parent->file path, instead of just the file key.
  • Do static analysis to detect unused/used variables and use this info for tree shaking

Current issues/decisions:

  • Packages global variables: today, when adding a new meteor package, ex: meteor/logs, it will end up polluting the global scope and it's not even necessary to import it to use it's exported symbols. Solution: Only make tree shaking available for "es6" ready meteor packages, they will have to signal the package as side effects free, and all imports/exports will have to be done.
  • Missing bare files from client/compatibility folders.
  • "undefinedundefined" symbol on dynamic imports - it was caused by source maps missing after removing symbols
  • Aliases causes loop on some modules

Pending Decisions:

  • Only run it on production builds? If we have a major "wrong" import, the local development build can have bad performance, or not be responsive at all. Think about import @material-ui/icons, for example.

Results until now:

  • importing from lodash-es is getting only what is imported, ex: import { add } from 'lodash-es', is bundling only the add.js file and the add() function.

@sebakerckhof
Copy link
Contributor

Won't all packages need to keep the side effects for backwards compatibility?

@renanccastro
Copy link
Contributor Author

Won't all packages need to keep the side effects for backwards compatibility?

Yes, that's the idea @sebakerckhof. We will keep packages as sideEffects: true, unless specified with api.setSideEffects(false) inside package.js

@sebakerckhof
Copy link
Contributor

sebakerckhof commented Sep 10, 2020

@renanccastro But is there a difference between api.sideEffects(false) and not having any api.export() 's ? And if not, does it make sense to introduce the additional call ? Since that would also imply existing packages which might be side-effect free already need to be updated before benefiting from tree-shaking.

@renanccastro
Copy link
Contributor Author

@renanccastro But is there a difference between api.sideEffects(false) and not having any api.export() 's ? And if not, does it make sense to introduce the additional call ? Since that would also imply existing packages which might be side-effect free already need to be updated before benefiting from tree-shaking.

Yes, I think so. Even if you don't specify an api.exports() and your package doesn't have a global namespace, when including it, Meteor will add it to the bundle. If this file has side effects, aside from adding the namespace, things can go wrong. But it's less common, I agree. It's more or less the way package.json has sideEffects on webpack, but here we are certain that when we see an api.exports(), sideEffect is true. When we don't see it, it can be false/true.

鈥se of a proxy export, now files are also "tree shaked", as well as export declarations. Working with lodash-es.
鈥s required avoiding cycles. It was missing symbols on import scanner.
@renanccastro
Copy link
Contributor Author

renanccastro commented Aug 23, 2021

@make-github-pseudonymous-again
Good news is that this use-case is also covered by this PR:
image

  import {enUS, nlBE, fr} from 'date-fns/locale'
  console.log(enUS);

It is only including enUS nlBE and fr code.

It's showing a 96,7% reduction in bundle size!


Your question about why we include package.json in the bundle can be answer by looking at the install module, which is our module system in the client side: https://github.com/benjamn/install#usage

We use it for identifying entry points in the client, but for sure, there is something odd in your picture. There is no way this package.json is this large, so I would guess it's something in bundle visualizer graph not updating accordingly.

@make-github-pseudonymous-again
Copy link

What's the status of conditionals on Meteor.isServer? Here is an example https://forums.meteor.com/t/idiomatic-method-client-simulation-opt-out/56546/7

@renanccastro
Copy link
Contributor Author

renanccastro commented Aug 30, 2021

Hi @make-github-pseudonymous-again
This PR also solves this, but changing this variables in bundle time and removing the unused code with terser minifying. Ex on the server:

	if (Meteor.isServer -> becomes true) {
		Meteor.methods({
			[name]: serverImplementation,
		});
	}
	else {
		Meteor.methods({
			[name]: clientImplementation,
		});
	}

Then, the else is removed when minifying.

@appigram
Copy link

appigram commented Nov 8, 2021

Hi guys, are there any blockers to finish it still? I read the discussion about build improvements #11587, And I think if it brings improvements for current projects already, can we see it in the next release? 馃槆

@make-github-pseudonymous-again
Copy link

Hi @make-github-pseudonymous-again This PR also solves this, but changing this variables in bundle time and removing the unused code with terser minifying. Ex on the server:

	if (Meteor.isServer -> becomes true) {
		Meteor.methods({
			[name]: serverImplementation,
		});
	}
	else {
		Meteor.methods({
			[name]: clientImplementation,
		});
	}

Then, the else is removed when minifying.

I am also interested in hoisting the client/server dichotomy. What about this example code, would it shake? Do we need to drop in some @@@inline@@@ hints to rollup (not sure those exist?) in case define is exported from its own module and imported in multiple places? Does it work with/without explicit Meteor import?

import {Meteor} from 'meteor/meteor';
const define = ({server, client}) => {
    return Meteor.isServer ? server : Meteor.isClient ? client : undefined;
};
export const x = define({server: () => { console.log('server'); }, client: () => { console.log('client'); });

Rationale: this allows more elegant abstractions than having to shove everything under a Meteor.isServer conditional. The code sample above is obviously oversimplified but I think it captures the indirection I think tree-shaking could have problems with.

@make-github-pseudonymous-again

Your question about why we include package.json in the bundle can be answer by looking at the install module, which is our module system in the client side: https://github.com/benjamn/install#usage

We use it for identifying entry points in the client, but for sure, there is something odd in your picture. There is no way this package.json is this large, so I would guess it's something in bundle visualizer graph not updating accordingly.

I just checked, the package.json file for @babel/runtime is really that large.

@tcastelli
Copy link
Contributor

Would be good to get a summary of the status of this one @renanccastro! Would really benefit from tree-shaking for many alread-covered cases 馃憤

@renanccastro
Copy link
Contributor Author

The bulk work here is done, but it impacts speed in dev mode.
What we have pending:

  • Only run it on production builds as there was a series of improvements on Reify making it possible to omit tree-shaking in dev (like this one)
  • Support custom compiler and custom file formats, like (.graphql) - issue here

@simplecommerce
Copy link

@renanccastro stupid question, how can I test this feature in my project to see the impact it does on the bundle size when building? I haven't done it before so I am not sure how.

@jankapunkt
Copy link
Contributor

What's the status of this PR?

@Saeeed-B
Copy link

Saeeed-B commented Jun 9, 2022

What's the status of this PR?

?

@denihs
Copy link
Contributor

denihs commented Jun 16, 2022

Hi, sorry for the delay here. I had to check with Renan to be sure of the status here, and it's still this #11164 (comment).

This is still in our roadmap, we didn't forget about it, but it's paused at the moment.

@jankapunkt
Copy link
Contributor

thank you @denihs I think it's totally okay to omit it in dev mode, thanks to recent improvements and if someone wants to test it, they can still use the --production flag, right?

@radekmie
Copy link
Collaborator

The only downside I can imagine is a potential tree-shaking bug, that won't come up during tests. (We're not the only ones running tests without --production, right?)

@jankapunkt
Copy link
Contributor

Good point, that could indeed be an issue.

@denihs
Copy link
Contributor

denihs commented Jun 17, 2022

Yeah... that's a downside that we'll have to look into it. But for now, you should be able to test it with --production.

@welkinwong
Copy link

What's the status of this PR鈥︹

@taochu
Copy link

taochu commented Aug 15, 2022

@jamauro
Copy link
Contributor

jamauro commented Jan 20, 2023

Would love to see this finalized and merged

@jamauro
Copy link
Contributor

jamauro commented Nov 8, 2023

Understandably the focus is on finishing 3.0, but I'm wondering if there's any chance this could be merged in parallel? I think the main outstanding decision is:

Only run it on production builds? If we have a major "wrong" import, the local development build can have bad performance, or not be responsive at all. Think about import @material-ui/icons, for example.

Maybe it could be an opt-in for now, e.g. --treeshake, that way people could test it.

The other issue linked in Renan's comment has since been closed.

If merging is not planned for a while, is there a way to test the PR as is?

@denihs @Grubba27 @fredmaiaarantes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet