Skip to content
This repository was archived by the owner on Mar 17, 2023. It is now read-only.

Conversation

@vovakulikov
Copy link
Contributor

Close https://github.com/sourcegraph/sourcegraph/issues/20772

Important This PR should be merged together with https://github.com/sourcegraph/sourcegraph/pull/21288

This PR changes fetching logic in a way to support partial commits loading in case if a user sets a big-time step (years) for their insight and got the moment on the chart when a particular repository hasn't been created yet.

Now we just skip these data points and show data that matter from the repo lifetime.

image

@vovakulikov vovakulikov force-pushed the code-insights/commits-timeline-problem branch from d832589 to 6a5e093 Compare May 24, 2021 15:43
"lnfs-cli": "^2.1.0",
"mkdirp": "^1.0.4",
"parcel-bundler": "^1.12.4",
"prettier": "^2.3.0",
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 also added prettier and prettier script to align code style in this repo

@felixfbecker
Copy link
Contributor

Two questions I have:

  1. Does this change the return value/type of the extension in any way?
  2. Is this change backwards-compatible with older Sourcegraph versions, i.e. this new extension version will continue to work on old instances?

@vovakulikov
Copy link
Contributor Author

@felixfbecker answered these questions here
https://github.com/sourcegraph/sourcegraph/pull/21288#issuecomment-849682928
But this thread seems more acceptable for this discussion about extension API so

Does this change the return value/type of the extension in any way?

Yes and no, we've changed datum value but from types perspective, nothing has been changed since we have datum as a generic in our types

 export interface LineChartContent<D extends object, XK extends keyof D> {
        chart: 'line'

        /** An array of data objects, with one element for each step on the X-axis. */
        data: D[]

        /** The series (lines) of the chart. */
        series: {}[]

        xAxis: ChartAxis<XK, D>
    }

Is this change backward-compatible with older Sourcegraph versions, i.e. this new extension version will continue to work on old instances?

The null value is a valid data point for our current visx charts but I'm not sure about old charts (re-chart library) I believe we still have these charts in sourcegraph 3.26. Do you have any suggestions on how can we publish a major update of extension for sourcegraph 3.27 and higher?

@felixfbecker
Copy link
Contributor

Major updates don't exist for extensions, it's always just one latest version that needs to be backwards-compatible to roughly the last 2 versions.

If this turns out to cause errors, what if we just returned 0 here (which feels semantically correct) and change it to null in a future release?

@vovakulikov
Copy link
Contributor Author

@felixfbecker I've thought about it, but using zero doesn't seem to be enough. The problem here is how can we separate situations when series got bad commits and then we should render non-active background and when series got valid commits but with zero values and we should not render non-active background but actually SVG line with zero values.

Also using zero values for bad commits is bad because we can break the visual representation of the chart in case if other series have a big Y-value. All series would be collapsed if we add zero value to one of the other series.

@vovakulikov
Copy link
Contributor Author

needs to be backward-compatible to roughly the last 2 versions.

it seems like these changes are suitable for this policy. Our re-charts insights were available in 3.26. We released new charts in 3.27 so 3.27 and 3.28 work with these major null-changes here.

But I understand the possible problem here. @felixfbecker wdyt perhaps we can add an experimental flag for this new behavior with null and enable it by default. If our customers have any problems with the backward compatibility of this extension, they can disable this logic.

@vovakulikov
Copy link
Contributor Author

@felixfbecker I checked re-charts with null values and zero values do not break the line charts of re-chart so it seems we are completely backward compatible with 3.26

https://codesandbox.io/s/simple-line-chart-forked-8ze5k

@felixfbecker
Copy link
Contributor

Awesome! I have no issues then and I don't think we need a flag. But we should wait for a code review from @valerybugakov (or someone else, if he can't get to it right now)

@felixfbecker felixfbecker removed their request for review May 27, 2021 19:17
@vovakulikov vovakulikov requested review from eseliger and tjkandala May 31, 2021 14:06
Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for improving the infrastructure of this repo.
One note: it would easier to review if this PR has two separate commits:

  1. For the infra upgrade.
  2. For the actual logic change.

We're guilty of the same thing in the frontend platform team :)
Please, ping us if you see any of our PRs/commits could be split further to ease the review process.

@vovakulikov vovakulikov merged commit f366753 into master Jun 1, 2021
@vovakulikov vovakulikov deleted the code-insights/commits-timeline-problem branch June 1, 2021 16:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

5 participants