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

Move management interface to its own package #7399

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

raksooo
Copy link
Member

@raksooo raksooo commented Dec 23, 2024

This PR moves the gRPC code generation for the management interface into its own npm package. This makes the file structure, build process and usage cleaner and easier to follow.


This change is Reviewable

Copy link

linear bot commented Dec 23, 2024

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

So nice!! 🔥 🙌

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


desktop/packages/management-interface/package.json line 6 at r1 (raw file):

  "author": "Mullvad VPN",
  "license": "GPL-3.0",
  "description": "",

⛏️ Could we add a quick description of this module? The mullvad-management-interface crate has the following description

Mullvad VPN IPC. Contains types and functions for IPC clients and servers.

Maybe we could write something similar here? 😊

Code quote:

"description": "",

@raksooo raksooo force-pushed the move-generated-grpc-code-to-separate-package-des-1587 branch from a12e71d to a9cbba1 Compare January 7, 2025 09:19
Copy link
Member Author

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 12 of 13 files reviewed, all discussions resolved (waiting on @MarkusPettersson98)


desktop/packages/management-interface/package.json line 6 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ Could we add a quick description of this module? The mullvad-management-interface crate has the following description

Mullvad VPN IPC. Contains types and functions for IPC clients and servers.

Maybe we could write something similar here? 😊

Done.

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@raksooo raksooo force-pushed the move-generated-grpc-code-to-separate-package-des-1587 branch from a9cbba1 to 703693b Compare January 7, 2025 13:57
olmoh
olmoh previously approved these changes Jan 7, 2025
Copy link
Collaborator

@olmoh olmoh left a comment

Choose a reason for hiding this comment

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

Great job! 🔥

Reviewed 12 of 13 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @raksooo)


desktop/packages/management-interface/src/index.ts line 1 at r2 (raw file):

export * from '../dist/management_interface_grpc_pb';

"Exporting from dist feels a bit unusual. An alternative would be to keep the generated files checked into source so they can be referenced directly. However, from what I understand other parts of the repo follow this pattern, and if so I think we should keep it consistent.

@raksooo raksooo dismissed stale reviews from olmoh and MarkusPettersson98 via 22d9356 January 7, 2025 15:41
@raksooo raksooo force-pushed the move-generated-grpc-code-to-separate-package-des-1587 branch from 703693b to 22d9356 Compare January 7, 2025 15:41
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @raksooo)

@raksooo raksooo force-pushed the move-generated-grpc-code-to-separate-package-des-1587 branch from 22d9356 to 5ea072d Compare January 7, 2025 15:59
Copy link
Member Author

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

I had to make some changes to packages/management-interface/package.json to make it all work on Windows.

Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98 and @olmoh)

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @raksooo)

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