add format "full-json" to Plotly.toImage and Plotly.dowloadImage

[antoinerg]

First step needed to complete plotly/orca#283

This PR introduces a new export format full-json that returns the graph JSON by using plots.graphJson()

Also, this PR introduces a version.js file containing the current version of the library which is easy to import in any part of the code. See 6a8ac65 for details.

TODO

• resolve conflicts
• sort object keys
• use full.json extension

cc @nicolaskruchten

[archmoj]

Since the figure attributes change from one version to another, we should possibly write plotly.js version to the output, IMO.

[etpinard]

we should possibly write plotly.js version to the output, IMO.

Not a bad idea!

Should we write it side-by-side with the data and layout attributes or put it in layout.version?

[antoinerg]

Should we write it side-by-side with the data and layout attributes or put it in layout.version?

Shouldn't we also consider config? Regardless, I think it would make sense to include the version.

cc @alexcjohnson

[etpinard]

Shouldn't we also consider config

We could, but in general the config won't be JSON-serializable. We could include only the JSON-serializable parts of the config and maybe add a placeholder for functions, for example:

modeBarButtonsToAdd: [{
  name: 'my button',
  click: function(gd) { console.log('hello') }
}]

// would become

{
  modeBarButtonsToAdd: [{
    name: 'my button',
    click: '_function_'
  }]
}

[archmoj]

One more question:
Is the resulting object sorted by keys?

[alexcjohnson]

Yes, I'd include config using @etpinard's suggestion for functions. And very nice idea @archmoj re: version. I'd say it goes at the top level, to make it clear it's not actually an attribute.

[antoinerg]

@etpinard is it OK to assume nothing in data or layout is ever a function?

If that's the case, can I simply replace

plotly.js/src/plots/plots.js

Lines 2054 to 2056 in 15d75db

	if(typeof d === 'function') {
	return null;
	}

with

        if(typeof d === 'function') {
            return '_function_';
        }

That should be safe and non-breaking, right?

[etpinard]

is it OK to assume nothing in data or layout is ever a function?

No, there are plenty of functions inside fullLayout e.g. fullLayout.xaxis.l2p

[nicolaskruchten]

I think we’d want to only include the parts of full* that are in the schema...

On Wed, Feb 19, 2020 at 17:31 Étienne Tétreault-Pinard < ***@***.***> wrote: is it OK to assume nothing in data or layout is ever a function? No, there are plenty of functions inside fullLayout e.g. fullLayout.xaxis.l2p — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4593?email_source=notifications&email_token=AABRWA54QL3RBFLA3DKBN5DRDWXLDA5CNFSM4KYB7PW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMJ6NLI#issuecomment-588506797>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABRWAY5YZXBFZ6SH27QCRDRDWXLDANCNFSM4KYB7PWQ> .

-- Nicolas Kruchten-VP Product Email nicolas@plot.ly 5555 Gaspe Ave #118, Montreal, QC, H2T 2A3

[alexcjohnson]

plots.graphJson already removes attributes with leading underscores and values that are functions. That covers everything outside the schema that's in data and layout. config converting to gd._context is handled differently and as @etpinard points out has some functions in the actual user-provided values, so sending it back will need special handling as well.

[nicolaskruchten]

I'm not sure about the value of returning config here TBH...

[alexcjohnson]

config could certainly be omitted at least to start - could always add it in later, perhaps if and when we convert it to using the same coerce machinery as data and layout.

[antoinerg]

@alexcjohnson adding config is not too bad (see 9497ed5)

The problem I have at the moment is outputting the version number (ie. Plotly.version) without getting circular dependency (see 9497ed5#r37388098). What's the easiest way to refer to the version number from anywhere in the code?

[etpinard]

What's the easiest way to refer to the version number from anywhere in the code?

The trick is to hardcode it just like in

plotly.js/src/core.js

Line 12 in 15d75db

exports.version = '1.52.2';

and

plotly.js/src/assets/geo_assets.js

Line 15 in 15d75db

exports.version = '1.52.2';

and then add a line below

plotly.js/tasks/preprocess.js

Lines 10 to 13 in 15d75db

	makeBuildCSS();
	copyTopojsonFiles();
	updateVersion(constants.pathToPlotlyCore);
	updateVersion(constants.pathToPlotlyGeoAssetsSrc);

updateVersion(/* path/to/file/ */);

and let the update_version.js util do the rest.

[antoinerg]

At the moment, when downloading the file via downloadImage we end up with a filename with extension .full-json. Should we instead set the extension to .full.json or .json?

cc @plotly/plotly_js @nicolaskruchten

[etpinard]

nicely done !!

[nicolaskruchten]

I like .json but this is a weakly-held preference.

[archmoj]

some TODOs here:

• resolve conflicts
• sort object keys
• use .json extension

[antoinerg]

• sort object keys

I'm wondering if sorting the object's keys recursively is really required? Would it be valuable to users?

cc @alexcjohnson @nicolaskruchten @archmoj

[alexcjohnson]

Sorting isn’t a hard requirement, could be omitted for now, but I do think it could be useful. @archmoj pointed out diffing, though I guess _full* will have a consistent order anyway due to the coerce process. But regardless, some objects will be pretty big, sorting will make it easier to find things.

[antoinerg]

Sorting isn’t a hard requirement, could be omitted for now, but I do think it could be useful.

Ok so would 81b8813 be satisfactory? I guess I could turn the forEach into a for loop. Any other comments?

[alexcjohnson]

Ok so would 81b8813 be satisfactory?

Looks great to me - I wouldn't worry about for vs forEach there, these are not looping over data arrays, and are not being called often.

No other comments from me, let's do it!

[alexcjohnson]

💃 beautiful!