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

Remix example #443

Closed
wants to merge 28 commits into from
Closed

Remix example #443

wants to merge 28 commits into from

Conversation

IgnisDa
Copy link
Contributor

@IgnisDa IgnisDa commented Dec 8, 2022

As discussed in #429, here is a functioning example for the remix framework. It features server side authentication and works with javascript disabled (except the login route of course).

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Dec 13, 2022

@lfleischmann Could you please review this?

@lfleischmann
Copy link
Member

lfleischmann commented Dec 16, 2022

@lfleischmann Could you please review this?

Hi @IgnisDa,

Yes, finally found time to get my eyes on this, sorry for the delay. And: thanks for taking the time to contribute the example!

Currently, I'm having problems with running the example in our compose cluster (I use the command you added to the "examples" directory readme). After a successful registration I get an error when loading the todo route:

SyntaxError: Unexpected token U in JSON at position 0
    at JSON.parse (<anonymous>)
    at Response.json (/app/node_modules/@remix-run/web-fetch/src/body.js:162:15)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at loader2 (/app/app/routes/todo.tsx:26:18)
    at Object.callRouteLoaderRR (/app/node_modules/@remix-run/server-runtime/dist/data.js:41:16)
    at callLoaderOrAction (/app/node_modules/@remix-run/router/router.ts:2638:14)
    at async Promise.all (index 0)
    at loadRouteData (/app/node_modules/@remix-run/router/router.ts:2306:19)
    at queryImpl (/app/node_modules/@remix-run/router/router.ts:2110:20)
    at Object.queryRoute (/app/node_modules/@remix-run/router/router.ts:2061:18)

This does not happen in a non-docker context but I am not entirely sure why right now. I am also having trouble to properly debug the remix application inside a container (any tips?).

I also noticed the remix application (remix dev server?) continuously makes websocket requests to the express app at port 8002 in the compose context. This also does not happen when running the services/applications in a non-docker context. Any idea why this happens?

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Dec 17, 2022

@lfleischmann I made some changes to the container. Now the running app uses the build output rather than the default remix dev server. So no WS connections are made any more.

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Dec 23, 2022

@lfleischmann Could you please take a look?

Copy link
Member

@lfleischmann lfleischmann left a comment

Choose a reason for hiding this comment

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

Hi @IgnisDa,

again, sorry for the delay, only just now getting back in the game after the holidays...

App runs fine now in the compose context, thanks. Just noticed one other thing: sometimes the web component does not properly render in its entirety/styles are not applied properly (see screenshot below for an example from Safari) and I have to reload to get a proper rerender. I can't reliably reproduce this though, sometimes it happens, sometimes it does not. So far seen in Chrome, Edge and Safari. I also checked whether this
is influenced by the shadow DOM usage (i.e. the shadow option on component registration) but it happens both with and without shadow DOM. Do you have any ideas why this is happening/how to fix this?

Screenshot 2023-01-11 at 12 50 09

examples/remix/app/routes/_api/logout.tsx Show resolved Hide resolved
@IgnisDa
Copy link
Contributor Author

IgnisDa commented Jan 11, 2023

I am not sure about the CSS thing, but I have encountered it too. I think it is related to remix-run/remix#2570 but can not be sure. It does not happen in incognito windows (i.e. when browser extensions are disabled). I have not been able to reproduce it reliably enough to be actually fix it.

@lfleischmann
Copy link
Member

I am not sure about the CSS thing, but I have encountered it too. I think it is related to remix-run/remix#2570 but can not be sure. It does not happen in incognito windows (i.e. when browser extensions are disabled). I have not been able to reproduce it reliably enough to be actually fix it.

Alright, thanks. We would like to investigate a bit further to find out what exactly causes the issue and how we can solve it to ensure the component is always rendered properly. It does not have top priority right now though, so I can't say how soon we are gonna tackle this.

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Jan 15, 2023

Okay makes sense. Will this PR be merged till then?

@like-a-bause
Copy link
Collaborator

Hey,
thanks for your effort. Unfortunately a sometimes working example does not pass our Quality Standards. Until this issue is resolved we won't merge this PR.

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Jan 26, 2023

@like-a-bause @lfleischmann I updated the remix packages, and I did not encounter the above bug anymore. Could you give it another spin and see if the problem persists?

@cayblood
Copy link

cayblood commented Feb 4, 2023

@IgnisDa OK, after updating REMIX_APP_TODO_API in .env to 8001 instead of 8002, the connection refused error is gone, but I'm still getting an error like this after generating a passkey and supplying the emailed code:

Screenshot 2023-02-03 at 10 52 01 PM

@cayblood
Copy link

cayblood commented Feb 4, 2023

@IgnisDa the error apparently is happening in /webauthn/registration/finalize:

{"time":"2023-02-04T17:24:28.338156423Z","time_unix":"1675531468","id":"1L4QmqpiJAnqjqTiVruGGleFk8waKQLh","remote_ip":"172.19.0.1","host":"localhost:8000","method":"POST","uri":"/webauthn/registration/finalize","user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36","status":400,"error":"failed to validate attestation","latency":7451459,"latency_human":"7.451459ms","bytes_in":742,"bytes_out":56,"referer":"http://localhost:64906/"}

I'll try to debug further.

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Feb 4, 2023

@cayblood Can you share your .env file? I will try to debug as well.

@cayblood
Copy link

cayblood commented Feb 4, 2023

@IgnisDa Here is my .env:

REMIX_APP_HANKO_API=http://localhost:8000
REMIX_APP_TODO_API=http://localhost:8001

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Feb 4, 2023

Here is mine

REMIX_APP_HANKO_API=http://localhost:8000
REMIX_APP_TODO_API=http://localhost:8002

@cayblood
Copy link

cayblood commented Feb 4, 2023

@IgnisDa Right. I had to change REMIX_APP_TODO_API to get the app to even kind of work. Prior to that I was getting a connection refused error as I commented earlier.

@cayblood
Copy link

cayblood commented Feb 4, 2023

@IgnisDa By the way, I'm also getting intermittent CSS errors. Sometimes I see:

Screenshot 2023-02-04 at 10 30 58 AM

And other times I see:

Screenshot 2023-02-04 at 10 30 52 AM

I can keep refreshing and randomly see one of those two results.

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Feb 4, 2023

Yep this is the reason why this PR has not been merged yet. Do you have the latest commit checked out?

@cayblood
Copy link

cayblood commented Feb 4, 2023

@IgnisDa Looks like a change just came in today. I was testing this last night. I've updated to the latest.

I did finally get it to work, but by ignoring the instructions in the README. Rather than running npm run dev, I noticed that the docker compose step already started a remix app on http://localhost:8888. I was able to browse to that address and successfully authenticate with the remix app. Running npm run dev ended up starting an app at a very high random local port number, like 63xxx. Browsing there presented me with the login prompt but always resulted in a failure to authenticate at the final step.

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Feb 4, 2023

Yep the docker-compose.yml file starts up all the required servers. After it is up and running (it takes the hanko backend about 30s to boot up), you can go to http://localhost:8888 to visit the remix app.

@cayblood
Copy link

cayblood commented Feb 4, 2023

@IgnisDa I would recommend updating the README inside the remix folder so that it accurately explains how to start and access the app.

@cayblood
Copy link

cayblood commented Feb 4, 2023

@IgnisDa looks like when remix-run/remix#4175 gets resolved the CSS problem will probably go away.

Update: that issue links to this one: facebook/react#24430. Seems like it is a problem with React 18 that has been experienced in other frameworks besides just Remix. Doesn't look like that is a high priority for the React team, based on how long the issue has been reported without a response. Might be worth pursuing a workaround if possible.

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Feb 5, 2023

I would recommend updating the README inside the remix folder so that it accurately explains how to start and access the app.

@cayblood Thanks, I updated the instructions.

Might be worth pursuing a workaround if possible.

I am aware of these issues, but have not been able to find any workarounds yet.

@cayblood
Copy link

@IgnisDa I found a potential workaround described here. Looking into it.

@cayblood
Copy link

@IgnisDa I submitted a PR to your fork with an easy workaround that falls back to React 17 functionality. I'll let you know if I come up with something better.

@IgnisDa
Copy link
Contributor Author

IgnisDa commented Feb 14, 2023

@lfleischmann @like-a-bause With @cayblood's help, we got the example fully working. I think it can be merged now.

// `.client.ts` files are only supposed to run on the client. I could not find this
// documented on the remix docs but someone on the remix discord told me this is how it is
// supposed to work.
import { register } from '@teamhanko/hanko-elements';
Copy link
Member

Choose a reason for hiding this comment

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

With this, the remix build command currently results in: "ERROR: Could not resolve "@teamhanko/hanko-elements"

Reverting to @teamhanko/hanko-elements@0.0.17-alpha requires the "old" import path for the register function, i.e. import { register } from '@teamhanko/hanko-elements/hanko-auth'.

Choose a reason for hiding this comment

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

@IgnisDa I just submitted a PR for this fix plus another compatibility issue I found with the ClientOnly component.

Choose a reason for hiding this comment

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

@IgnisDa @lfleischmann replied below before my fix was merged into this PR. I think their concerns would go away and we could finally get this merged with that fix. That said, we should probably update to 0.5.0, now that it has been released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added you as a collaborator in my repository in case you need to make any changes.

@lfleischmann
Copy link
Member

@lfleischmann @like-a-bause With @cayblood's help, we got the example fully working. I think it can be merged now.

Hi @IgnisDa, @cayblood, once again: thank your for your time and effort! Unfortunately, we're still seeing the problem with unapplied styles in Safari, so we're holding off the merge for now until we're sure the example works across browsers.

@IgnisDa IgnisDa closed this by deleting the head repository May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants