-
Notifications
You must be signed in to change notification settings - Fork 111
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
RSDK-899 import proto methods from typescript-sdk #1601
RSDK-899 import proto methods from typescript-sdk #1601
Conversation
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.
😍
@@ -41,16 +42,6 @@ tool-install: | |||
gotest.tools/gotestsum \ | |||
github.com/rhysd/actionlint/cmd/actionlint | |||
|
|||
buf: buf-web | |||
|
|||
buf-web: tool-install |
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.
reminder to self, will need to add sessions proto to sdk instead of using buf here
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
Makefile
Outdated
@@ -21,7 +20,9 @@ build: build-web build-go | |||
build-go: | |||
go build $(GO_BUILD_TAGS) ./... | |||
|
|||
build-web: buf-web | |||
build-web: | |||
npm ci --audit=false |
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.
Do we still need to install/maintain any npm
packages at the root level of the project? That single dependency was for how we previously generated buf
stuff.
I imagine we can delete the package.json
file at the root and just keep the one in web/frontend
.
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.
great call -- yes the top-level dependency is only for generating protobufs, which this PR removes
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.
✅ just a question about dependencies
02f33c5
to
dba79ac
Compare
|
JIRA: https://viam.atlassian.net/browse/RSDK-899
Stop generating JS protobufs and instead import them from our typescript sdk.
Questions:
web/runtime-shared
now?TODOs