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.

Sign up for GitHub

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

Jump to bottom

fixes for gleam 0.33 #8

Closed
wants to merge 5 commits into from
Closed

fixes for gleam 0.33 #8

wants to merge 5 commits into from

Conversation

gdamjan
Copy link

@gdamjan gdamjan commented Dec 21, 2023

syntax changes to update to latest Gleam

  • import types with import a/b.{type C}

  • pub external type -> pub opaque type

  • BitString -> BitArray

  • pub external fn -> @external(erlang, …

  • update dependencies, especially gleeunit 0.10 was incompatible with gleam 0.33

@gdamjan
fixes for gleam 0.33
1b10f95 
- update dependencies, especially gleeunit 0.10 was incompatible with
  gleam 0.33

syntax changes to update to latest Gleam
- import types with import a/b.{type C}
- pub external type -> pub opaque type
- BitString -> BitArray
- pub external fn -> @external(erlang, …
@gdamjan
Copy link
Author

gdamjan commented Dec 21, 2023

tests don't pass though:

   Compiled in 0.23s
    Running nerf_test.main
F
Failures:

  1) nerf_test.echo_test
     Failure: undef
     stacktrace:
       gun.ws_send
       nerf/gun.ws_send
       nerf_test.echo_test
       nerf_test.echo_test
     output: 

Pending:
  nerf_test.echo_wss_test
    %% Unknown error: {timeout,
                   #{stacktrace =>
                         [{gun,await_up,3,
                              [{file,
                                   "/home/damjan/src/nerf/build/dev/erlang/gun/src/gun.erl"},
                               {line,838}]},
                          {gun,await_up,1,
                              [{file,
                                   "/home/damjan/src/nerf/build/dev/erlang/gun/src/gun.erl"},
                               {line,820}]},
                          {nerf@websocket,'-connect/4-fun-4-',3,
                              [{file,
                                   "/home/damjan/src/nerf/build/dev/erlang/nerf/_gleam_artefacts/nerf@websocket.erl"},
                               {line,53}]},
                          {nerf_test,echo_wss_test,0,
                              [{file,
                                   "/home/damjan/src/nerf/build/dev/erlang/nerf/_gleam_artefacts/nerf_test.erl"},
                               {line,165}]},
                          {eunit_test,'-mf_wrapper/2-fun-0-',2,
                              [{file,"eunit_test.erl"},{line,273}]},
                          {eunit_test,run_testfun,1,
                              [{file,"eunit_test.erl"},{line,71}]},
                          {eunit_proc,run_test,1,
                              [{file,"eunit_proc.erl"},{line,543}]},
                          {eunit_proc,with_timeout,3,
                              [{file,"eunit_proc.erl"},{line,368}]}]}}
  undefined
    %% Unknown error: {blame,[1,2]}


Finished in ? seconds
4 tests, 1 failures, 3 cancelled

@gdamjan
bit_builder.from_bit_string replaced with gleam/bytes_builder from_bi…
b0ed07b 
…t_array
@gdamjan
gun:ws_send requires the StreamRef
fcf8fba 
and so `nerf_ffi:ws_send_erl` will need it too.
this is a change from gun 2.0

so move `ws_send/2` and `ws_send_erl` in src/nerf/websocket.gleam and pass it
the whole Connection (not just ConnectionPid)

- remove websocket.Frame, there's already gun.Frame.
- fix test to use gun.Frame type
@gdamjan
Copy link
Author

gdamjan commented Dec 21, 2023

ws:// tests now pass with the podman/docker image.

wss:// on socketsbay.com are a bit problematic, since it seems there's a lot of garbage on those sockets. looking into some wss:// echo services now.

@gdamjan gdamjan marked this pull request as ready for review December 28, 2023 14:23
@rockerBOO rockerBOO mentioned this pull request Jan 5, 2024
Open
lpil
lpil reviewed Jan 7, 2024
View reviewed changes
src/nerf/gun.gleam
external fn ws_send_erl(ConnectionPid, Frame) -> OkAtom =
"nerf_ffi" "ws_send_erl"

pub fn ws_send(pid: ConnectionPid, frame: Frame) -> Nil {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this function has been mistakenly removed?

Copy link
Author

Choose a reason for hiding this comment

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

individual commits had a bit more context.

ws_send/2 and ws_send_erl were moved in in src/nerf/websocket.gleam since they require Connection (not just ConnectionPid).

Now another approach would be to split the types into their own file, so we can import them everywhere, or … circular imports (are they allowed?)

Copy link
Owner

Choose a reason for hiding this comment

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

Please revert this breaking change, thank you

lpil
lpil reviewed Jan 7, 2024
View reviewed changes
src/nerf/websocket.gleam
Close
Text(String)
Binary(BitString)
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this has been mistakenly removed?

Copy link
Author

Choose a reason for hiding this comment

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

type Frame exists as gun.Frame already. it was not clear if it was intentional to have two separate types with the same name?

Copy link
Owner

Choose a reason for hiding this comment

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

The API is intentionally designed, the documentation explains the goals.

Copy link
Author

Choose a reason for hiding this comment

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

The API is intentionally designed, the documentation explains the goals.

maybe I'm missing something. Can you point me to the documentation so I can understand this better.

https://hexdocs.pm/nerf/ is a bit sparse

Copy link
Owner

Choose a reason for hiding this comment

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

They are deliberately different. The nerf module is a low level binding.

lpil
lpil reviewed Jan 7, 2024
View reviewed changes
Copy link
Owner

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Hello! The upgrade looks good but please revert the changes to the API. Thank you

@jluzny
Copy link

jluzny commented Mar 30, 2024

I tested the latest upgrade in my fork of glome, and it seems to be working just fine :)

@lpil
Copy link
Owner

lpil commented Mar 30, 2024

Thanks @jluzny but this is PR is unfinished presently.

@gdamjan gdamjan closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpil lpil lpil left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gdamjan @jluzny @lpil