-
Notifications
You must be signed in to change notification settings - Fork 11
feat: download json component #866
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #866 +/- ##
==========================================
- Coverage 85.41% 85.35% -0.07%
==========================================
Files 796 798 +2
Lines 16352 16393 +41
Branches 2056 2057 +1
==========================================
+ Hits 13967 13992 +25
- Misses 2353 2369 +16
Partials 32 32
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
</div> | ||
` | ||
}) | ||
export class DownloadJsonComponent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it could be easily renamed to be a generic download component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now, this component does not support downloading other 'resources' like files etc. It only supports downloading a JavaScript data object as a JSON. Either we take care of all cases or rename this component to be more specific to its use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It currently takes any string or object and supports downloading it as a file, which is the only download we'll likely have since we're using GQL. If we get new use cases in the future, can always expand supported inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, i was thinking we can build a wrapper around this, which we can call 'ht-download'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary, IMO. This is the only download mechanism we'll need, the only issue is that it won't always be json and that's already supported in the implementation as is.
} | ||
|
||
private downloadData(data: string): void { | ||
const dlJsonAnchorElement: HTMLAnchorElement = document.createElement('a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please inject Renderer2 and document to interface with the DOM (or far better, move this logic into the template if feasible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this should be present in the template with all the required url and attributes so that Users can open it in a new tab.
const dlJsonAnchorElement: HTMLAnchorElement = document.createElement('a'); | ||
const downloadURL = `data:text/json;charset=utf-8,${encodeURIComponent(data)}`; | ||
dlJsonAnchorElement?.setAttribute('href', downloadURL); | ||
dlJsonAnchorElement?.setAttribute('href', downloadURL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dupe line
}) | ||
export class DownloadJsonComponent { | ||
@Input() | ||
public dataSource!: Observable<unknown>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data as an observable seems weird to me. This has potential side effects too. For example, If I pass a GraphQl query response observable and you subscribe on it (like you are doing below) it will fire a new query.
Why not just use the data directly? You can do
<trace-download-json
*ngIf="this.data$ | async as data"
[data]="data"
</trace-download-json>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that better strictly from an api perspective, but from a functional one the issue there is it's going to force a download every time we show an icon, which we don't want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With respect to exporting Trace as Json feature, we can either optimize on space by fetching only when user clicks on the button or we can optimize on time by fetching in advance utilizing the idle browser/network/server cycle (in the background)
In the former case, the download prompt will probably start after 2-3 sec (or more) depending upon the query. In the latter case, we are only using an idle browser and network cycle and download is immediate.
With my suggestion, there are two problems:
- Hypothetically, if we have multiple download buttons then we are downloading too much data in advance
- If the first fetch query fails, Users would not get a second try since the button wouldn't show up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the former case, the download prompt will probably start after 2-3 sec (or more) depending upon the query. In the latter case, we are only using an idle browser and network cycle and download is immediate.
- It's not an idle cycle, since it would be downloading on page load when we're trying to download the page's main data
- 99 times out of 100 (random numbers! but honestly, I think its actually >99) this is not going to be used. Exporting data is not a main use case, it's for debugging (or moving data into a different tool), and pre-downloading each time is just wasteful - both for the client mentioned above, but also for the server. And like you said, as we start adding downloads elsewhere in the product, we're forcing all of them to pre-download.
- That 2-3 seconds seems like a pretty big overestimate to me, but I guess depends on connection and the trace. Either way, having that button show up delayed after we do the download rather than immediately (but taking time to download once clicked) is just worse UX to me, and contrary to how downloads work in pretty much any other product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
By idle cycle I mean, this process will not affect or block the rendering of the main contents on the page. We do load widgets in a dashboard in a similar fashion. This will be another async call right?
-
Only with this Trace JSON feature, we are downloading a data which is not already displayed in the UI. Contrary to this, all other usage for this button should always be using the data which is displayed in the associated table/list. So a separate network shouldn't be required.
-
I agree that this may still be used very occasionally.
-
Yes. the delay was some rough estimate which depends on connection/load/trace.
-
I don't really agree with your points regarding the ux being bad for button showing up with delay. We do this currently at so many places. It is not going to cause any flicker or anything. By the time user look for it, it will be there. We can also show this with loader. (Please refer to Blocked events sheet for a similar ux)
That being said, I am fine with downloading on click time. The only concern I have is to avoid any unintended network call with any subscription inside this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we address that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By idle cycle I mean, this process will not affect or block the rendering of the main contents on the page. We do load widgets in a dashboard in a similar fashion. This will be another async call right?
Every network call we make, while yes async, will slow down others that are active, right? making 3 calls in parallel is faster than making 4. The tubes are only so big!
Only with this Trace JSON feature, we are downloading a data which is not already displayed in the UI. Contrary to this, all other usage for this button should always be using the data which is displayed in the associated table/list. So a separate network shouldn't be required.
Future use cases presumably may have to fetch the data needed to download, too. Some cases it may be downloading the same data that's already displayed, but certainly case by case.
I don't really agree with your points regarding the ux being bad for button showing up with delay. We do this currently at so many places. It is not going to cause any flicker or anything. By the time user look for it, it will be there. We can also show this with loader. (Please refer to Blocked events sheet for a similar ux)
We could put it there with a loader, but those cases are really places where we need to load. For example, dropdowns, data panels etc. that are all reliant on data even if you don't interact with them.
The only concern I have is to avoid any unintended network call with any subscription inside this component.
I'm not positive what you mean by this, but if you mean the fact that it would make a new network call on each click, that's not necessarily wrong and is again how almost every download button in the world works - if I download something once, then click the download button again, it will just try to download it again (another network call). That said, the component's parent can easily decide by sharing or not sharing a passed in observable whether it wants that behavior or not - the beauty of observables!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every network call we make, while yes async, will slow down others that are active, right? making 3 calls in parallel is faster than making 4. The tubes are only so big!
There is not much UI rendering involved with this call. If we profile the page, i am confident the overall UI response time would only go down fractionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not positive what you mean by this
refer to test resubscribing to a debounced query after execution triggers a debounced new request
for GraphQlRequestService
.
Example, If we are using this on events table, I would share the same data$ with this component. Since there is a subscription from within this component, it would likely trigger a secondary network call. (Unless I remember to share the observable inside the parent component)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not much UI rendering involved with this call. If we profile the page, i am confident the overall UI response time would only go down fractionally.
It's not the UI rendering, it's the network bandwidth - you can only transmit so many bits, no matter how many calls you make.That said, I don't disagree that it's likely a minimal perceived impact - but still a waste of both client and server resources.
Example, If we are using this on events table, I would share the same data$ with this component. Since there is a subscription from within this component, it would likely trigger a secondary network call. (Unless I remember to share the observable inside the parent component)
If the parent wants to use the same observable in two places but only fetch once, its on the parent to share it - no one else can make that decision. Taking in a sync string for example and the parent using an async pipe like you suggested above *ngIf="this.data$ | async as data"
would dupe the call too - except it would dupe it on every parent load, rather than just if the user clicked download. All of it is a bit moot considering these calls are going to be cached in apollo, but if anything it's another argument towards deferring the download IMO.
This comment has been minimized.
This comment has been minimized.
@itssharmasandeep For test failures, follow the approach used in https://github.com/hypertrace/hypertrace-ui/blob/main/projects/components/src/copy-to-clipboard/copy-to-clipboard.component.test.ts . Also, you can probably avoid creating new anchor tag element everytime. This would also simplify your tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping aside the ongoing discussions about component API, rest of the changes LGTM.
Please address the comments as next iteration and file a ticket to keep track of it.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This comment has been minimized.
This comment has been minimized.
Unit Test Results 4 files 253 suites 15m 43s ⏱️ Results for commit 2189e8a. |
private readonly changeDetector: ChangeDetectorRef, | ||
private readonly notificationService: NotificationService | ||
) { | ||
this.dlJsonAnchorElement = this.document.createElement('a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this an anchor in the template? could we wrap the button in an anchor so the user is actually clicking the anchor they're triggering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since here we're using an observable to fetch the data, so it is not possible to do that.
We need the data before click, So that's why this is the approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we don't wrap it (which I still think is doable), having an anchor in this template that's not displayed is more or less equivalent to what we're doing here, so is definitely feasible (basically just changes the createElement to populate that to a @ViewChild
)
public fileName: string = 'download'; | ||
|
||
@Input() | ||
public tooltip: string = 'Download Json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove tooltip as indicated on the other PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would do in other PR
'href', | ||
`data:text/json;charset=utf-8,${encodeURIComponent(data)}` | ||
); | ||
this.renderer.setAttribute(this.dlJsonAnchorElement, 'download', `${this.fileName}.json`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please take the extension in as part of the file name as indicated on the other PR
import { NotificationService } from '../notification/notification.service'; | ||
|
||
@Component({ | ||
selector: 'ht-download-json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still like to see this component be renamed as it has nothing json-specific.
import { DownloadJsonComponent } from './download-json.component'; | ||
import { DownloadJsonModule } from './download-json.module'; | ||
|
||
describe('Button Component', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update test describe name
|
||
describe('Button Component', () => { | ||
let spectator: Spectator<DownloadJsonComponent>; | ||
const mockElement = document.createElement('a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will go away if we put the anchor into the template
Description
Download json component
Testing
Local testing done.
Checklist:
Download button

When downloading in progress (On click)
