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

Viddy as an importable package. #77

Closed
wants to merge 1 commit into from
Closed

Viddy as an importable package. #77

wants to merge 1 commit into from

Conversation

anvial
Copy link
Contributor

@anvial anvial commented Sep 15, 2022

This PR should allow importing Viddy tool as a lib in other Golang projects.

Also, I've added the PreconfiguredNewViddy func to avoid additional dependencies in 3-party projects.

I suppose this is a good structure to use both:

  • as a library
  • as a binary

Ready for comments and discussions.

@gedw99
Copy link

gedw99 commented Sep 15, 2022

thanks @anvial

The project has a dependency on the TUI lib. I am planning to use viddy in a WASM project where it is cross compiled using tinygo and then run it in the browser. So I was wondering if the project can be structured so that the TUI and the core viddy are separated ?

cc @sachaos

@sachaos
Copy link
Owner

sachaos commented Sep 15, 2022

@gedw99 It is interesting that you are going to run it in WASM.
I have a few questions. (I don't have any WASM experience, so sorry if I'm wrong.

  • What would be the use case in WASM? viddy executes a command and outputs it, but what would it output instead?
  • If we don't depend on the TUI library, what will we depend on instead?

"github.com/gdamore/tcell/v2"
"github.com/moby/term"
"github.com/rivo/tview"
"github.com/spf13/viper"
Copy link
Owner

Choose a reason for hiding this comment

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

To reduce the dependency of the viddy lib, I think it will be great if we can remove dependency of the viper from lib.
What do you think?

Copy link

Choose a reason for hiding this comment

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

+1 on removing viper from the lib. assume it will stay in the TUI ... ?

@gedw99
Copy link

gedw99 commented Sep 15, 2022

Hey @anvial excellent questions and thanks for asking. Let me know if this explains things better ...

  • What would be the use case in WASM? viddy executes a command and outputs it, but what would it output instead?

Service workers inside a browser are essentially http servers. You can write them in golang and compile them to wasm. So they are processes, just like you have processes on your laptop.

I am building a quasi IDE in golang, and want to use viddy in exactly the same way. This quasi IDE is in golang, and compiled to run as a GUI.

The same is also true on the server side. Checkout https://github.com/tetratelabs/wazero.
You can compile your golang to wasm, and get instant starts and sandboxing.
Viddy can then used with wazero to inspect what is going on with the golang / WASM running on a local or remote server.

  • If we don't depend on the TUI library, what will we depend on instead?

What i propose is to have viddy as a golang package. Then any GUI / TUI can import it and use it.

I am using https://github.com/gioui to build the GUI that does what the current TUI does.
It can run on Web, Desktop and Mobiles. Its 100% golang code just like the TUI.

@sachaos
Copy link
Owner

sachaos commented Sep 15, 2022

@gedw99 Thank you.
It is nice to remove the dependency and user can change the view.

But currently, viddy depends on rivo/tview (TUI lib) heavily.
And I think it would be hard.
I don't think removing rivo/tview should be done in this PR.

If you want to remove rivo/tview from viddy lib.
Feel free to submit the PR.

@gedw99
Copy link

gedw99 commented Sep 15, 2022

I don't think removing rivo/tview should be done in this PR.

@sachaos your right. Its best to do in later PR.

I will make an issue now for it so we can swing back to it later.

here is the Issue: #78

@anvial
Copy link
Contributor Author

anvial commented Sep 15, 2022

@sachaos,

what is your feedback about current PR? What do you think about changing the structure?

I also need your advice and suggestion on how to implement the --version flag, because looks like this PR has broken it...

@sachaos
Copy link
Owner

sachaos commented Sep 15, 2022

@anvial About structure, please refer to my idea of importable package of viddy.
(This is WIP and not working, but the interface would be that.
https://github.com/sachaos/viddy/blob/feature/refactoring-viddy/pkg/viddy/viddy.go
I'm happy if you like this and follow that in this PR.

About, version and help.
Please parse os.Args by newConfig() in main.go, and handle that in main.go.
Then pass the Config struct to NewViddy().
Then the viddy lib package will no longer depends on viper.

@anvial
Copy link
Contributor Author

anvial commented Sep 15, 2022

@sachaos

https://github.com/sachaos/viddy/blob/feature/refactoring-viddy/pkg/viddy/viddy.go

Your approach looks clean and smooth. I think this is the right way to implement it.

How can we help the refactoring-viddy branch?

@sachaos
Copy link
Owner

sachaos commented Sep 15, 2022

@anvial If you already have viddy on the juju side, making viddy importable may not be an urgent issue. In that case, I would appreciate it if you could wait for my work.

If there are other requests and it should be importable, I think it is possible to imitate only the viddy interface of that branch and leave the content unrefactored on this PR.
Then, even if I refactor later, the interface should not change, so I think that the users of the library will not be bothered.

@anvial
Copy link
Contributor Author

anvial commented Sep 15, 2022

That's reasonable,

Thx so much for your work!

PS: Looking forward to switching the sachaos/viddy repo after the needed functionality is introduced.

@anvial anvial closed this by deleting the head repository Sep 20, 2022
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