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

Download razor telemetry nupkg and include in vsix #7236

Merged
merged 10 commits into from
Jun 18, 2024

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Jun 14, 2024

@ryzngard ryzngard requested review from a team as code owners June 14, 2024 20:26
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

It's not clear to me why we need two different packages, or to put things in two different folders, it seems like we're probably just making our install size bigger by duplicating DLLs. But thats not really a reason to block this PR so have a green tick :)

tasks/offlinePackagingTasks.ts Outdated Show resolved Hide resolved
@ryzngard
Copy link
Contributor Author

It's not clear to me why we need two different packages, or to put things in two different folders, it seems like we're probably just making our install size bigger by duplicating DLLs. But thats not really a reason to block this PR so have a green tick :)

I don't have a strong opinion here. the duplicate dll is just the telemetry dll, and technically we may be on a separate version than Roslyn. If we decide to require the same version then we could probably reduce size slightly

@davidwengier
Copy link
Contributor

I was thinking duplicated dlls between our two nupkgs, like shared code or workspaces.DLL maybe. If there isn't any, then great, ignore me :)

Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

or to put things in two different folders

For us it's a nice way to ensure they don't even accidentally load in non-devkit scenarios if they're in a different folder.

};

export function getComponentPaths(componentName: string, options: LanguageServerOptions): string[] {
export function getComponentPaths(componentName: string, options: LanguageServerOptions | undefined): string[] {
Copy link
Member

Choose a reason for hiding this comment

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

instead of having this undefined you could consider adding the razorDevkit folder as a component path option. Then you would be able to override it for local development.

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 think the only issue I have with that is the structure LanguageServerOptions has a lot of concepts that don't apply to razor. Could we move the options to a lighter interface?

Copy link
Member

Choose a reason for hiding this comment

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

I guess technically the other options aren't relevant to you, but you can just import languageServerOptions and pass it to the function (without caring what else is on it). Actually, frankly builtInComponents.ts should probably just call languageServerOptions.componentPaths and not even have options be a parameter.

Copy link
Member

Choose a reason for hiding this comment

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

We could move it to CommonOptions as well, but not sure if its necessary to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, frankly builtInComponents.ts should probably just call languageServerOptions.componentPaths and not even have options be a parameter

I'll look into that. Seems cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually let me see about that in a follow up. Would like to get this in for now since I'll likely have another razor change coming in that will require dual insertion

@ryzngard ryzngard merged commit a296353 into dotnet:main Jun 18, 2024
13 checks passed
@ryzngard ryzngard deleted the razor_devkit branch June 18, 2024 21:15
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.

3 participants