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

Custom Port #34

Merged
merged 2 commits into from
Feb 3, 2025
Merged

Custom Port #34

merged 2 commits into from
Feb 3, 2025

Conversation

floaterest
Copy link
Contributor

Passing --static-file-host 127.0.0.1:0 as arguments to the typst preview binary will start the preview local server on a random port. This PR adds an option to the config that allows a user-specified port instead of a random one.

resolves #33

@chomosuke
Copy link
Owner

Hello there, thanks for this PR. I'll be happy to merge this PR if you add the port option to README's default config also. And please explain that 0 means any free port. Please also handle the error of when the port is already taken by notifying the user with utils.notify(...).

@floaterest
Copy link
Contributor Author

Sure, I will work on that

floaterest added a commit to floaterest/typst-preview.nvim that referenced this pull request Jul 18, 2024
floaterest added a commit to floaterest/typst-preview.nvim that referenced this pull request Jul 18, 2024
@floaterest
Copy link
Contributor Author

Sorry for the delay.

I implemented the error handling and added documention (in README.md and typst-preview.nvim.txt)

Copy link
Owner

@chomosuke chomosuke left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the delay as well, I did not notice that you've completed this PR.
I will be happy to merge this PR if you can somehow handle the error of if the port is already in use by another process. I believe you might need to inspect the output by the typst-preview binary to achieve this.

Thank you

lua/typst-preview/servers/manager.lua Outdated Show resolved Hide resolved
@floaterest
Copy link
Contributor Author

Hi, I have updated the codebase.

It will now detect the "AddrInUse" error from tinymist. To handle the error, the code will attempt to use the next port (e.g. try 8001 instead of 8000).

I have also updated the docs and README.

@floaterest floaterest requested a review from chomosuke January 6, 2025 23:51
@LEXUGE
Copy link

LEXUGE commented Feb 1, 2025

Hi, can we maybe not try "port +1" by default or at least have an option to disable it? Instead can we return an error so that user can handle the error by themselves if they prefer.

Copy link
Owner

@chomosuke chomosuke left a comment

Choose a reason for hiding this comment

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

LGTM except some early return that I subjectively did not like.

@chomosuke
Copy link
Owner

Hi, can we maybe not try "port +1" by default or at least have an option to disable it? Instead can we return an error so that user can handle the error by themselves if they prefer.

@LEXUGE Please open a new issue requesting this. This PR has been there for a while and i'd like to merge it instead of requesting more changes.

@chomosuke chomosuke merged commit 00ff682 into chomosuke:master Feb 3, 2025
@chomosuke
Copy link
Owner

And thank you for excellent work :>.

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.

Feature Request: Custom Preview URL Port
3 participants