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

[DISCUSSION] Referencing chart.js through our library #95

Closed
Joelius300 opened this issue Mar 29, 2020 · 6 comments
Closed

[DISCUSSION] Referencing chart.js through our library #95

Joelius300 opened this issue Mar 29, 2020 · 6 comments
Labels
breaking change Implementing this feature will cause a breaking change and can only be done in a major release. discussion For various discussions
Milestone

Comments

@Joelius300
Copy link
Contributor

Joelius300 commented Mar 29, 2020

In the README and in our samples, we reference chart.js and moment.js through our library with the _content directory added by blazor. It's included in the sources of this library and is automatically integrated into the client-side root of applications that consume this library.
But why?

We can write what version of chart.js we currently support and we can let the consumer chose themselfs how to include the chart.js. We can also let them chose if they want the minified version or not. Why should we include the JavaScript-file of chart.js in our library as well, doesn't it just bloat the nuget size?

It would be a breaking change to remove it but here's why I think it would make sense.

Pro

  • Smaller nuget-size
  • We can keep the type files for TypeScript, no need to change that
  • When we migrate to chart.js 3.0, we'll have trouble with the time-adapters anyway. You don't need moment.js anymore, you can use any time-adapter you want. So which one would we have in our library?
  • More flexible for the consumer (minified, version, cdn)
  • Only the script tag in the _Host.cshtml or index.html would change in the consumers application

Contra

  • Breaking change, consumers need to change some code
  • We need to clearly document what version we support and be aware of it (this is actually a benefit IMO but it requires additional work)

If possible, I would like to hear some opinions on this before we decide anything. Can anyone tell us some good arguments for keeping it in? Or do you agree with me that it should be excluded from the project and just be documented well?

cc @mariusmuntean

@Joelius300 Joelius300 added the discussion For various discussions label Mar 29, 2020
@SeppPenner
Copy link
Contributor

SeppPenner commented Mar 29, 2020

The most important point is:

We need to clearly document what version we support and be aware of it (this is actually a benefit IMO but it requires additional work)

If this is done properly, the users won't have disadvantages through the change.

My problem with this is always that I as a customer wouldn't like to change these things as long as the library works properly. I mean, it's a bit more configuration and not just "install the nuget and it works" if you now what I mean :) But, of course, I understand why some people might find this useful.

@Joelius300
Copy link
Contributor Author

I get your point but is it really more configuration?

Right now consumers have to put this in their client-side entry point:

<script src="_content/ChartJs.Blazor/Chart.min.js"></script>

After we remove that chart.js from our sources, they'll have to put the following instead:

<script src="https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.9.3/Chart.min.js"></script>

Is that really a difference? In both cases, we just put it in the readme and people copy paste it into their applications, only very few will actually think about what version makes the most sense etc.

And when we put it in the readme, we can even put it with SRI, it'll still be copy paste. Just installing the nuget won't be enough either way.

<script src="https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.9.3/Chart.min.js" integrity="sha256-R4pqcOYV8lt7snxMQO/HSbVCFRPMdrhAFMH+vr9giYI=" crossorigin="anonymous"></script>

I still think it would be an improvement over the current solution, people can still keep a local copy if they want to.

@SeppPenner
Copy link
Contributor

You're right. There's no more configuration needed, my mistake :)

I just don't really like CDNs, but the script can be added locally either way, so there's no issue.

@Joelius300
Copy link
Contributor Author

Yea if you don't want to use a CDN or only a certain one, you can do it. But if you do that right now, the js-file will still get copied around even though you don't need it. That's what I'm trying to eliminate.

@Joelius300 Joelius300 added the breaking change Implementing this feature will cause a breaking change and can only be done in a major release. label Apr 9, 2020
@Joelius300 Joelius300 added this to the 2.0.0 milestone Apr 9, 2020
@Joelius300
Copy link
Contributor Author

Closing this because I recently removed the pre-built Chart.js code and also all moment.js stuff. When I release 2.0, I'll update the readme to use a CDN link with a notice that you can download the latest builds from the Chart.js page if you don't want to use a CDN. I think that's a good way to handle it.

@SeppPenner
Copy link
Contributor

@Joelius300 Yeah, this seems like a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Implementing this feature will cause a breaking change and can only be done in a major release. discussion For various discussions
Projects
None yet
Development

No branches or pull requests

2 participants