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

Update release build to not include externals. #84

Merged
merged 3 commits into from Apr 5, 2017
Merged

Update release build to not include externals. #84

merged 3 commits into from Apr 5, 2017

Conversation

ghost
Copy link

@ghost ghost commented Mar 29, 2017

This PR is an enhancement proposal focusing on the removal of all external deps (vue, graph.js, ...) from the final build and will also reduce minified build size from 500 kB to 59.1 kB.

@codecov
Copy link

codecov bot commented Mar 29, 2017

Codecov Report

Merging #84 into develop will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           develop    #84   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           10     10           
  Lines           85     85           
======================================
  Hits            85     85

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94dcc0f...129a52e. Read the comment docs.

@apertureless
Copy link
Owner

Thanks for the PR. 👍

I did not tested it and I did not worked with peerDependencies so far, but as far as I understand, you will need to install the peerDependenciesseparately right?

So we would go from yarn add vue-chartjs to yarn add vue-chartjs chart.js vue.js lodash right?

Which would also be a breaking change. So I am not sure, if it's a good idea.
It would also break the browser version for people that are using it over an CDN without a build process (webpack, rollup, browserify)

Vue.js

I think it is a good idea add Vue.js as a peerDependency as you will mostly have vue installed if you want to work with vue-chartjs. Prior I could not remove Vue as a dependency, because I am using Vue.extend() in the *.vue files. Which was kind of a pain, because of multiple vue instances and version mismatch. And will also reduce the bundle size.

Lodash

But I don't think that seperating lodash as an external is necessary. I am using only the merge from the fp package.

import merge from 'lodash/fp/merge'

Which is not that big and forcing people to install lodash is imo too much hassle. Furthermore it could get nasty for people without a building process (unkpg directly in the browser).

Chart.js

I am not sure, what to do with Chart.js. Right now I would not see a benefit in adding it as a peerDependency. Because if you want to work with vue-chartjs you mostly will not have installed Chart.js as a dependency. So you would be forces to install chart.js and vue-chartjs which is kind of duplicated right?

The bundle size of vue-chartjs would decrease a lot but in the end, it would be the same, as you have to install chart.js no matter what.

What are your thoughts on that? ☕️

@ghost
Copy link
Author

ghost commented Mar 31, 2017

I did not tested it and I did not worked with peerDependencies so far, but as far as I understand, you will need to install the peerDependenciesseparately right?

Yes.

So we would go from yarn add vue-chartjs to yarn add vue-chartjs chart.js vue.js lodash right?

Probably more something like yarn add charts.js vue-chartjs, vue will be already installed most of the time (as for lodash please refer to my point below).

Which would also be a breaking change.

Yes, a couple of UNMET PEER DEPENDENCY will start to show in your terminal buffer.
However, this is mainly a change for npm/yarn users. Documentation should also be updated.

It would also break the browser version for people that are using it over an CDN without a build process (webpack, rollup, browserify)

Maybe an additional type of distribution is necessary here (full v.s standalone?).

I think it is a good idea add Vue.js as a peerDependency as you will mostly have vue installed if you want to work with vue-chartjs. Prior I could not remove Vue as a dependency, because I am using Vue.extend() in the *.vue files. Which was kind of a pain, because of multiple vue instances and version mismatch. And will also reduce the bundle size.

Maybe using extends as object attribute (https://vuejs.org/v2/api/#extends) or the install function to access the Vue instance (https://vuejs.org/v2/guide/plugins.html) is a solution for you here.

I am using only the merge from the fp package.

In that specific case, I definitely agree that making lodash an external might not be the best thing to do.
https://github.com/lodash/lodash-webpack-plugin also exists for that purpose, not sure I'll use it in this context, though.

I am not sure, what to do with Chart.js. Right now I would not see a benefit in adding it as a peerDependency. Because if you want to work with vue-chartjs you mostly will not have installed Chart.js as a dependency.

vue-chartjs make use of chart.js in the same way that babel-loader make use of babel-core (https://github.com/babel/babel-loader/blob/master/package.json#L16).

End-user can choose over a peer dependency specific version (at their own risk and with a whinny warning "vue-chartjs@^x.x.x" has incorrect peer dependency "chart.js@^x.x.x". each time npm or yarn is used, but still) or simply install the supported one : freedom.

@apertureless
Copy link
Owner

Maybe an additional type of distribution is necessary here (full v.s standalone?).

Yeah that sounds good.

Maybe using extends as object attribute (https://vuejs.org/v2/api/#extends) or the install function to access the Vue instance (https://vuejs.org/v2/guide/plugins.html) is a solution for you here.

Well this will not work. You can later extend the base charts this way. But for the components I have to use the Vue.extend. However this should also work with vue as a peerDependency.

End-user can choose over a peer dependency specific version (at their own risk and with a whinny warning "vue-chartjs@^x.x.x" has incorrect peer dependency "chart.js@^x.x.x". each time npm or yarn is used, but still) or simply install the supported one : freedom

I see. Makes sense for me.

Can you re-add lodash as a dependency? Like mentioned I don't think its nessasary to make it external, as I am only using the fp/merge and the hassle to install it addtionally will be greater then the bit smaller bundle size.

I will test it then and merge.

@apertureless apertureless self-requested a review April 3, 2017 17:52
Copy link
Owner

@apertureless apertureless left a comment

Choose a reason for hiding this comment

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

lodash needs to be add as a dependency

@@ -15,6 +15,11 @@ module.exports = {
libraryTarget: 'umd',
umdNamedDefine: true
},
externals: {
'vue': 'vue',
'lodash': 'lodash',
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove lodash from externals and re-add it as a dependency

@apertureless apertureless merged commit 6ac651e into apertureless:develop Apr 5, 2017
@apertureless
Copy link
Owner

@gcoguiec Thanks!

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.

3 participants