search

amcharts / export

Apache License 2.0
56 stars 33 forks source link

Upgrade PDF make #60

Closed Ilshidur closed 7 years ago

Ilshidur commented 7 years ago

The current pdfmake version is very outdated. For exemple, it exports the package lodash version 3.1.0, but the current pdfmake version uses the version 4.17.4 (package.json of pdfmake).

I also recommend to use the Bower dependency instead of simply include it in the repository.

martynasma commented 7 years ago

Hi there,

Sorry for the delay in response. We had an internal discussion about this.

The thing is that contents of this repo is used to auto-populate our downloadable packages as well as CDN servers. Getting rid of the libraries in favor of Bower dependencies would make life easier for those few who use Bower, but would introduce a lot of additional hassle for majority of people who don't.

Besides, since we don't have control over 3rd party libraries, we can't guarantee the new version would work. Included versions are guaranteed and tested to work, whereas with Bower dependencies we'd either need to constantly test and update dependency version restrictions, or just cap at the latest known working version, which would beat the purpose.

I hope you find this explanation reasonable.

Ilshidur commented 7 years ago

Hello and thank you for your answer.

I perfectly understand your opinion on Bower and I totally agree, according to your explanations.

I also would like to know about a possible -manual- update of the third party librairies, like pdfmake, that would prevent bugs, deprecations and/or just outdated libs.

martynasma commented 7 years ago

We tent to upgrade the version of the 3rd party library only in if it fixes a specific bug that directly affects Export plugin operation.

You're absolutely welcome to try out new versions of pdfmake and other libraries. The chacnes are good they will work. However, we don't have the capacity to test out new versions ourselves, I'm afraid.

Ilshidur commented 7 years ago

Thank you. I have one last question ...

Out of curiosity, admitting someone submit a -fully tested- PR with updated dependencies, what would the merging process be ?

martynasma commented 7 years ago

As mentioned before, we tend not to upgrade dependencies unless a) it solves a bug affecting Export or b) introduces a feature we would like to take advantage of.

I know it's tempting to grab the latest version. But sometimes it breaks things, often in a way that's not prominent at first. Most of our users do not have the requirement for those libraries other than the use with Export plugin, so upgrading them without significant reason is not advisable.

That being said, we do review all PRs, but there's no guarantee or approved process for merging it in, I'm afraid.

barocsi commented 6 years ago

you must upgrade because it simply throws an error Cannot read property 'TYPED_ARRAY_SUPPORT' of undefined (pdfmake v0.1.32) https://github.com/bpampuch/pdfmake/issues/1100

martynasma commented 6 years ago

@barocsi Which one throws the error? The one we have currently bundled? (0.1.29) or the one you mentioned? (0.1.32)?