Skip to content

Send event on referencing large file #26197

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

Merged
merged 1 commit into from
Aug 13, 2018
Merged

Send event on referencing large file #26197

merged 1 commit into from
Aug 13, 2018

Conversation

sheetalkamat
Copy link
Member

Event when referencing large file as per #26169 (comment)

@RyanCavanaugh RyanCavanaugh requested review from amcasey and removed request for amcasey August 3, 2018 22:51
@amcasey
Copy link
Member

amcasey commented Aug 3, 2018

If this were turning into telemetry (which I don't believe it is), the file size would be okay but the file path would not.

@amcasey
Copy link
Member

amcasey commented Aug 3, 2018

@armanio123, should VS respond to this event? Do we want to show an info message or something? This seems similar to LS disabled.

@armanio123
Copy link
Contributor

I think it might be a good idea to show an info message, just to make the user aware of what's happening on his project. Nevertheless, I don't think the feedback we have received relates to this scenario specifically or if we can get an action from the user by showing the message.

@sheetalkamat
Copy link
Member Author

Is this good to merge ?

@amcasey
Copy link
Member

amcasey commented Aug 10, 2018

I have no objections, but Ryan only added me by accident. I know @sandersn is adding an event, so he might have thoughts.
As I mentioned above, as long as this doesn't turn into telemetry, I have no GDPR objections.

@sheetalkamat
Copy link
Member Author

@RyanCavanaugh @sandersn Ok to merge ?

@sandersn
Copy link
Member

I’m interested in this PR as an example of how to add a new flag, but I’ll take a look at the other parts now.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good as far as I understand the code; just one question about code duplication.

@@ -2436,6 +2436,27 @@ namespace ts.server.protocol {
openFiles: string[];
}

export type LargeFileReferencedEventName = "largeFileReferenced";
Copy link
Member

Choose a reason for hiding this comment

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

Why is this code duplicated? I guess that’s one public and the other private, but why aren’t they shared?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's because we want to be able to build protocol.ts separately as a standalone to protocol.d.ts which editors consume.

@sheetalkamat sheetalkamat merged commit f2011ce into master Aug 13, 2018
@sheetalkamat sheetalkamat deleted the largeFileEvent branch August 13, 2018 18:14
@sandersn sandersn mentioned this pull request Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants