-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add widget api #1503
base: master
Are you sure you want to change the base?
Add widget api #1503
Conversation
bc86de2
to
07b912b
Compare
e1d4e3c
to
cb52d94
Compare
.github/workflows/publish-lib.yml
Outdated
{ | ||
echo '{ | ||
"name": "@bluerobotics/cockpit-api", | ||
"version": "'${{ github.ref_name }}'", |
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 think we should not use the cockpit version here. thoughts?
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.
We shouldn't. Cockpit has new releases every week, while the API will probably be updated every few months.
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 have put some comments here and there, but in general I really like the solution and loved that you added support for Ardupilot variables in the data-lake!
/** | ||
* Current version of the Cockpit Widget API | ||
*/ | ||
export const COCKPIT_WIDGET_API_VERSION = '0.0.0' |
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 should be injected in build time, right?
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.
yes. where should the "source of truth" live, though?
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.
Difficult.
If it was in its own repo, we could track the git release, but in this case we maybe have to hardcode it in the packages.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.
idk. we need it in 3 places: packages.json, publish-lib.yml, and api.ts 🤔
is it easier to move from code to packages.json or from packages.json to code?
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 thinking of leaving the source of truth on publish.yaml
then we can make it an environment variable for building, and use sed to inject it into packages.json.
I don't really like the idea, but I can't think of anything better. thoughts?
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.
Sounds valid. Can't think of something better either.
cb52d94
to
13312b8
Compare
13312b8
to
4d6333e
Compare
No description provided.