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

internal: remove package-level init with fastlyABIInit() #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Apr 15, 2023

We were ignoring the return value anyway and it complicates wizer integration.

@dgryski
Copy link
Member Author

dgryski commented Apr 15, 2023

Just need to figure out where to put this for people who want to call it. Maybe at the top-level sdk.go file ?

We were ignoring the return value anyway and it complicates wizer integration.
@dgryski dgryski force-pushed the dgryski/explicity-fastly-abi-init branch from 797c52d to e8d19a9 Compare April 15, 2023 19:01
@joeshaw
Copy link
Member

joeshaw commented Apr 18, 2023

What purpose does the call serve? What're the implications of not calling it?

@dgryski
Copy link
Member Author

dgryski commented Apr 18, 2023

The call allows an app to say "I'm using this version of the ABI" and verify that the wasm runtime supports it. However, it is purely advisory and the return value ("Sounds Good" / "I'm sorry I don't support that version") was ignored anyway.

@joeshaw
Copy link
Member

joeshaw commented Apr 19, 2023

What's the right thing to do if we don't support the version? Panic? If so, we could maybe put it into the fsthttp.ServeFunc function since that's going to be the entrypoint for nearly all apps.

@JakeChampion
Copy link
Contributor

What's the right thing to do if we don't support the version? Panic? If so, we could maybe put it into the fsthttp.ServeFunc function since that's going to be the entrypoint for nearly all apps.

I'd possibly also write to stderr so that something appears in fastly log-tail

@fgsch
Copy link
Member

fgsch commented Apr 21, 2023

Can we just move this into handle.go:Serve(), before we create the ClientRequest?

@dgryski
Copy link
Member Author

dgryski commented May 15, 2023

There are code entries that could be called before Serve(), for example loading a configuration or something from a kvstore or equivalent. So while it would be seen, it wouldn't necessarily be the first action the SDK would take with the host.

@fgsch
Copy link
Member

fgsch commented May 16, 2023

There are code entries that could be called before Serve(), for example loading a configuration or something from a kvstore or equivalent. So while it would be seen, it wouldn't necessarily be the first action the SDK would take with the host.

Ah, yes, you are right. I still think this should happen automatically (and not something the end user should care about) but I'm not sure how to achieve that outside init or adding the extra check in all the possible entrypoints.

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.

4 participants