Skip to content

migration: Add Global API Treeshaking #144

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

Merged
merged 7 commits into from
Jul 13, 2020
Merged

migration: Add Global API Treeshaking #144

merged 7 commits into from
Jul 13, 2020

Conversation

phanan
Copy link
Member

@phanan phanan commented Jul 12, 2020

No description provided.

}
```

This will tell webpack “Hey, treat this Vue module as an external library and don’t bother bundling it.”
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This will tell webpack “Hey, treat this Vue module as an external library and don’t bother bundling it.
This will tell webpack to treat this Vue module as an external library. As a result, it will not bundle it.

The dialogue is nice, but it seems like an abstraction rather than a simple statement which could confuse people not as familiar with English.

Copy link
Member Author

@phanan phanan Jul 13, 2020

Choose a reason for hiding this comment

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

Nice catch! At the same time, people not familiar with English might find "it will not bundle it" confusing though (I did during my early days). WDYT if I take your change and modify it a bit into:

This will tell webpack to treat this Vue module as an external library and not bundle it.

Copy link
Member

Choose a reason for hiding this comment

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

Looks great to me!

But what if you’ve never had to deal with manual DOM manipulation, nor are you using or testing async components in our app? Or, what if, for whatever reason, you prefer to use the good old `window.setTimeout()` instead? In such a case, the code for `nextTick()` will become dead code – that is, code that’s written but never used. And dead code is hardly a good thing, especially in our client-side context where every kilobyte matters.

Module bundlers like [webpack](https://webpack.js.org/) support [tree-shaking](https://webpack.js.org/guides/tree-shaking/), which is a fancy term for “dead code elimination.” Unfortunately, due to how the code is written in previous Vue versions, global APIs like `Vue.nextTick()` are not tree-shakeable and will be included in the final bundle regardless of where they are actually used or not.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Current Syntax

@@ -1,3 +1,160 @@
# Global API Treeshaking

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Previous Syntax

Copy link
Member

@bencodezen bencodezen left a comment

Choose a reason for hiding this comment

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

Added some headings to separate out the content and make it easy to tell what sections are relevant.

Do you think it would be possible to add a summary section at the beginning?

@bencodezen bencodezen mentioned this pull request Jul 13, 2020
25 tasks
Copy link
Member

@NataliaTepluhina NataliaTepluhina left a comment

Choose a reason for hiding this comment

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

Awesome work @phanan! I've added a few minor questions/nitpicks

Copy link
Member

@bencodezen bencodezen left a comment

Choose a reason for hiding this comment

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

@phanan Other than the small phrasing we talked about, I'm good to go on this too. Thanks for your hard work on this!

@NataliaTepluhina
Copy link
Member

@phanan thanks for the fixes! Changes look good to me now 👍

@phanan phanan merged commit 7ebaed9 into master Jul 13, 2020
@phanan phanan deleted the migration/treeshaking branch July 13, 2020 18:11
@phanan
Copy link
Member Author

phanan commented Jul 13, 2020

Thanks y'all!

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

Successfully merging this pull request may close these issues.

3 participants