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

[Issue #1260] Introducing support for GitHub Codespaces #1356

Closed
wants to merge 4 commits into from
Closed

[Issue #1260] Introducing support for GitHub Codespaces #1356

wants to merge 4 commits into from

Conversation

rahulbagai
Copy link

@rahulbagai rahulbagai commented Jul 17, 2023

This PR implements support for Github codespaces in Reflex. The modifications involve updating the constants.py file to include a condition. When the project is loaded using Github codespaces, the API_URL will automatically be set to an HTTPS endpoint. This improvement ensures seamless integration and enhances security for users.

Platforms used for testing include:

  • Github Codespaces
  • Local virtual devcontainer in VS Code.

One test is currently not passing, and it appears to be failing in the main branch as well. Do you have any thoughts or ideas on how to address this issue?

    def do_execute(self, cursor, statement, parameters, context=None):
>       cursor.execute(statement, parameters)
E       sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) near "DROP": syntax error
E       [SQL: ALTER TABLE alembicthing DROP COLUMN t1]
E       (Background on this error at: https://sqlalche.me/e/14/e3q8)

/home/vscode/.cache/pypoetry/virtualenvs/reflex-qu3Isagt-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/default.py:736: OperationalError

FAILED tests/test_model.py::test_automigration - sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) near "DROP": syntax error

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

After these steps, you're ready to open a pull request.

a. Give a descriptive title to your PR.

b. Describe your changes.

c. Put `closes #XXXX` in your comment to auto-close the issue that your PR fixes (if such).

@rahulbagai
Copy link
Author

closes #1260

@Alek99
Copy link
Member

Alek99 commented Jul 18, 2023

Nice will test this, thanks for taking this on!

@masenf masenf added the testing label Jul 18, 2023
@rahulbagai
Copy link
Author

rahulbagai commented Jul 25, 2023

@Alek99 @masenf Hey there! Just wanted to check if there's any update on this ticket. Please let me know if there's anything else I can do to make it easier for you. Thanks a bunch!

@masenf
Copy link
Collaborator

masenf commented Jul 31, 2023

Support for DROP COLUMN was added in sqlite 3.35, but the devcontainer mcr.microsoft.com/devcontainers/python:1-3.11-bullseye is using

>>> sqlite3.sqlite_version_info
(3, 34, 1)

That's why the test is failing. Let me try it with the bookworm variant which has

>>> sqlite3.sqlite_version_info
(3, 40, 1)

@masenf
Copy link
Collaborator

masenf commented Jul 31, 2023

@rahulbagai do you have an example of a working app running in a codespace?

when I try it, I can get the frontend to load, but it always fails to establish the websocket connection

RuntimeError: Expected ASGI message 'http.response.start', but got 'websocket.accept'.

@rahulbagai
Copy link
Author

@masenf Thanks for testing my changes! Unfortunately, I'm experiencing the same issue on my end as well. I couldn't find any recent changes in the repository that could help identify the problem. Is there any way I can check the server logs for more debugging? I'm fairly new to this repository, so any assistance would be greatly appreciated!

@masenf
Copy link
Collaborator

masenf commented Aug 1, 2023

@rahulbagai did you have it working "before" just wondering if there's some regression or if there is yet undiscovered changes needed to work in the codespaces environment.

i tried toggling between public and private port visibility and it didn't seem to make a difference. kind of smells like some sort of "middle box" isn't correctly handling the websocket requests...although the hmr websocket for hot reload on port 3000 does seem to work without an issue.

@rahulbagai
Copy link
Author

I had it working "before." I should have recorded a video or something to share for confirmation. Mind sharing your debugging steps? That way, I can give it a shot on my end too. Let's troubleshoot together!

@masenf
Copy link
Collaborator

masenf commented Aug 1, 2023

  1. Files Changed tab
  2. Review in Codespace
  3. Create new Codespace

In the codespace terminal I'm running the following commands

poetry install
poetry shell
mkdir /tmp/app
cd /tmp/app
reflex init
reflex run

Small window pops up noting that port 3000 is open, so I click the Open Browser button and the frontend loads fine. Looking in the dev tools, it continually attempts to connect to /event and gets a 200 code back, instead of 101 switching protocols

image

The API_URL seems correct: wss://masenf-sturdy-space-trout-5g6479p6p43xr5-8000.preview.app.github.dev/event/?EIO=4&transport=websocket, but it just cannot keep the connection open, like it doesn't want to upgrade the connection to a websocket.

image

@rahulbagai
Copy link
Author

@masenf, thanks a bunch for the detailed troubleshooting steps! I managed to reproduce the steps on my end too. By the way, I've made some progress on debugging and figured out a few other things.

Here's what I found during my testing:

  1. I ran the backend API and frontend separately, with debug logs enabled. During this, I encountered an exception on the API.
INFO: 50.47.218.247:0 - "GET /?EIO=4&transport=websocket HTTP/1.1" 500 Internal Server Error
ERROR: Exception in ASGI applicationRuntimeError: Expected ASGI message 'http.response.start', but got 'websocket.accept'.
  1. The exception signifies that when a client tries to establish a WebSocket connection, it sends an HTTP request with the Upgrade header set to websocket. The ASGI application should handle this and respond with a http.response.start message containing a 101 status code and websocket headers, followed by a websocket.accept message. However, in this case, the application is directly returning websocket.accept without the http.response.start, causing the server to respond with a 500 error.

I'm now diving into the ASGI API setup to see if there's anything else we need to enable for a successful WebSocket connection. Any insights you have would be greatly appreciated!

Just a few other things that come to mind, which I'm looking forward to debugging:

  1. Making sure the frontend and backend are using compatible versions of Reflex. Mismatched versions might cause some websocket protocol issues.
  2. There might be an issue during the websocket connection setup, but since I don't see any other errors besides the ones mentioned above, we'll need to investigate further.

@masenf
Copy link
Collaborator

masenf commented Aug 2, 2023

Possibly related to #698

@masenf masenf mentioned this pull request Aug 2, 2023
3 tasks
masenf added a commit that referenced this pull request Nov 3, 2023
@masenf
Copy link
Collaborator

masenf commented Nov 3, 2023

Rebased and extended in #2125, websockets are working now! 🎉

@masenf masenf closed this Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants