[Enhancement] JMC-6589 Bundle the d3 library for flame charts with JMC

[Jie Kang]

On Thu, Oct 24, 2019 at 2:53 PM Miro Wengner <miro.wengner at gmail.com> wrote:
>
> Hi Jie,
>  I’d normally agree with you, but:
> 1. In case we’d love to have it pulled out of the remote sources then we'd need to use npm or another front-end package build system.
> 2. What I also realized, during my work with “d3 libraries”, is that not all modifications are compatible with each other, could be an issue.
>
> maybe this solution is the lowest pain and effective.

Hey Miro,

Regarding point 1, maybe a third-party maven plugin to download
sources from a remote URL could be used if the minified js files are
hosted somewhere? I agree it would be a bit too much to add a
dependency on a full packaging system like npm.

Can you elaborate on the incompatible modifications for d3 libraries
you mention in point 2?


Regards,
Jie Kang

>
> What do you think?
>
> Thank you for looking into it !
>
> Kind Regards,
> Miro
>
>
>
> > On Oct 24, 2019, at 7:40 PM, Jie Kang <jkang at redhat.com> wrote:
> >
> > Hi Miro, Marcus,
> >
> > I think the contents are technically sound. I'm surprised to see the
> > minified js assets be added to version control of the jmc project
> > though. I see them as binary results of another project's build, so
> > this is akin to adding say a copy of the HdrHistogram jars to the vcs.
> > As dependencies, could it be made to have these files pulled from the
> > remote sources during the build process of the flame chart? What do
> > you think?
> >
> >
> > Regards,
> > Jie Kang
> >
> > On Tue, Oct 22, 2019 at 3:52 AM Miro Wengner <miro.wengner at gmail.com> wrote:
> >>
> >> Hi Everyone,
> >>  here is my updated version, please review my changes,
> >>
> >> issue: https://bugs.openjdk.java.net/browse/JMC-6589 <https://bugs.openjdk.java.net/browse/JMC-6589>
> >> webrev: http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.02/ <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.02/>
> >>
> >> Kind Regards,
> >> Miro
> >>
> >>> On Oct 21, 2019, at 1:42 PM, Marcus Hirt <marcus at hirt.se> wrote:
> >>>
> >>> Let's discuss #3 in the Slack a bit.
> >>>
> >>> Kind regards,
> >>> Marcus
> >>>
> >>> -----Ursprungligt meddelande-----
> >>> Från: jmc-dev <jmc-dev-bounces at openjdk.java.net> För Miro Wengner
> >>> Skickat: den 21 oktober 2019 13:28
> >>> Till: Marcus Hirt <marcus.hirt at datadoghq.com>
> >>> Kopia: jmc-dev at openjdk.java.net
> >>> Ämne: Re: [Enhancement] JMC-6589 Bundle the d3 library for flame charts with JMC
> >>>
> >>> Hi Marcus,
> >>> thank you for taking a look!
> >>> 1) “page.html.o <http://page.html.org/>rig” : should not be there, I’ve it overlooked -> removed, formatting too!
> >>> 2) I’d personally also prefer renaming -> page.html -> page.template
> >>> 3) I’d also go with placeholders in following case
> >>>
> >>> I’ll post a new update version
> >>> Kind Regards,
> >>> Miro
> >>>
> >>>
> >>>> On Oct 20, 2019, at 2:42 PM, Marcus Hirt <marcus.hirt at datadoghq.com> wrote:
> >>>>
> >>>> Might be better to just rename the page.html to page.template, and add placeholders for the place(s) you want to inject stuff.
> >>>>
> >>>> /M
> >>>>
> >>>> On Sun, Oct 20, 2019 at 12:14 PM Marcus Hirt <marcus.hirt at datadoghq.com <mailto:marcus.hirt at datadoghq.com>> wrote:
> >>>> Hi Miro,
> >>>>
> >>>> Thank you for looking into this. A few nits, after a very superficial look:
> >>>> * Why keep page.html.orig around? Is it still being used somewhere?
> >>>> * The comments you have for where you fetched the javascript bundles differ in that only the last one has a space between the beginning of the comment and the text. All of them should be in the same format (personally I like the space).
> >>>>
> >>>> And then a general comment:
> >>>> I guess inlining the Javascript into the HTML is one of the few options we have, but wouldn't you want to have a more "unique" string to go look for? Many of the keys you use are short and look like they, if we're unlucky, could perhaps be used in one of the libraries we include in the future. I'd suggest namespacing them with something like jmc.inline just to reduce the likelihood of any future collisions.
> >>>>
> >>>> Kind regards,
> >>>> Marcus
> >>>>
> >>>>
> >>>>
> >>>> On Fri, Oct 18, 2019 at 10:52 PM Miro Wengner <miro.wengner at gmail.com <mailto:miro.wengner at gmail.com>> wrote:
> >>>> Hi Everyone
> >>>> please review my enhancement for flamechart
> >>>>
> >>>> webrev:http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev/ <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev/> <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev/ <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev/>>
> >>>> bug:https://bugs.openjdk.java.net/browse/JMC-6589 <https://bugs.openjdk.java.net/browse/JMC-6589> <https://bugs.openjdk.java.net/browse/JMC-6589 <https://bugs.openjdk.java.net/browse/JMC-6589>>
> >>>>
> >>>> Kind Regards,
> >>>> Miro
> >>>
> >>>
> >>
> >
>