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

Drop transforms from the API #7240

Merged
merged 13 commits into from
Oct 24, 2024
Merged

Drop transforms from the API #7240

merged 13 commits into from
Oct 24, 2024

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 18, 2024

This is a big one:
Closes #7192 and closes #7190.

@plotly/plotly_js
cc: @etpinard

Comment on lines 1094 to 1095
fullTrace._fullInput = fullTrace;
fullTrace._expandedInput = fullTrace;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without transforms there will always be a 1:1 relationship between gd.data and gd._fullData entries, right? So we should be able to entirely get rid of _expandedIndex (just use index) as well as _fullInput and _expandedInput (just use _input)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Addressed in 89e2321.

@alexcjohnson If I understand correctly, you want me to remove gd._fullData and use gd.data everywhere instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Addressed in 89e2321.

@alexcjohnson If I understand correctly, you want me to remove gd._fullData and use gd.data everywhere instead?

That won't work. So the current state of the PR should be good enough.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry, got the substitution wrong. Looks like _expandedInput is already gone, but now since fullTrace._fullInput is always just fullTrace, shouldn't it be possible to remove it everywhere? ie when you have a line like

var iIn = inputIndices[i] = trace._fullInput.index;

it can turn into

var iIn = inputIndices[i] = trace.index;

And then there will be further simplifications because what you're creating is often now the same as something else, or the whole block is unnecessary like:

if(trace._fullInput !== trace) {
[].push.apply(fullData.selectedpoints, pointIndices);
}

because that would become if (trace !== trace)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in b22f62b.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, you should be able to remove _fullInput entirely at this point - not just in the cases where you're accessing _fullInput.index. This isn't causing any problems, it's just cruft we should remove at some point. If you want to move on and make an issue to come back to this later then sure, but here's what I see at the current state of this PR:

 ~/plotly/plotly.js/src > drop-transforms > ag _fullInput
traces/parcoords/plot.js
60:        var preGUI = fullLayout._tracePreGUI[gd._fullData[fullIndices[i]]._fullInput.uid];
119:        //     fullLayout._tracePreGUI[gd._fullData[fullIndices[i]]._fullInput.uid],

traces/table/data_preparation_helper.js
41:    var columnOrder = trace._fullInput.columnorder;

components/legend/handle_click.js
47:        fullTrace = fullTrace._fullInput;
89:        var fullInput = fullTrace._fullInput || fullTrace;
108:    var fullInput = fullTrace._fullInput;

components/legend/draw.js
113:                _fullInput: shape,
559:                var fullInput = legendItem.trace._fullInput || {};

components/selections/select.js
946:        var fullInputTrace = searchTraces[i].cd[0].trace._fullInput;
958:            trace._input.selectedpoints = trace._fullInput.selectedpoints = [];
959:            if(trace._fullInput !== trace) trace.selectedpoints = [];
970:                if(trace._fullInput !== trace) {
975:                if(trace._fullInput !== trace) {
985:            if(trace._fullInput !== trace) {
986:                delete trace._fullInput.selectedpoints;

plot_api/template_api.js
345:            if(!fullTrace._fullInput._template) {

plot_api/plot_api.js
1445:            var preGUI = fullLayout._tracePreGUI[getFullTrace(tracei)._fullInput.uid];
1517:            var preGUI = fullLayout._tracePreGUI[contFull._fullInput.uid];
2400:        if(fullData[i]._fullInput.uid === uid) return i;
2521:                fullInput = fullTrace._fullInput;
2784:            trace = newFullData[i]._fullInput;
2788:            getDiffFlags(oldFullData[i]._fullInput, trace, [], diffOpts);

plots/cartesian/type_defaults.js
121:    var isCandlestick = traceIs(trace._fullInput || {}, 'candlestick');

plots/plots.js
502:        uid = newFullData[i]._fullInput.uid;
554:        var thisFullInput = oldFullData[i]._fullInput;
1091:        fullTrace._fullInput = fullTrace;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to address this part in a separate PR also to move forward with QA and documentation for the RC release.

@gvwilson gvwilson added feature something new P1 needed for current cycle labels Oct 21, 2024
@archmoj archmoj requested a review from alexcjohnson October 21, 2024 16:54
@archmoj
Copy link
Contributor Author

archmoj commented Oct 23, 2024

@alexcjohnson Is there any remaining work needed on this PR?

@archmoj archmoj merged commit 056c8e7 into master Oct 24, 2024
1 check passed
@archmoj archmoj deleted the drop-transforms branch October 24, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

drop deprecated transforms in v3 drop deprecated calendar transforms in v3
3 participants