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

feat: remove strings dependency, optimizations #23

Merged
merged 6 commits into from
Jan 17, 2024
Merged

Conversation

nilslice
Copy link
Member

@nilslice nilslice commented Jan 2, 2024

Pulling in #16, just easier to start from a clean branch that has been rebased.
Thanks @syke99 for leading the effort to get this done!

@nilslice nilslice self-assigned this Jan 2, 2024
@nilslice nilslice requested a review from zshipko January 2, 2024 21:40
Comment on lines +6 to +7
GOOS=wasip1 GOARCH=wasm go build -tags std -o example/std_countvowels.wasm ./example/countvowels
GOOS=wasip1 GOARCH=wasm go build -tags std -o example/std_http.wasm ./example/http
Copy link
Member Author

Choose a reason for hiding this comment

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

added build tags to the files in example/ so we can keep these single-path locations for the builds and conditionally compile the right code based on which compiler is used.

the downside is we need to maintain two versions of the examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bhelx ^ this is one consequence but not a breaking change.

the library does have breaking changes but nothing really extreme

Comment on lines +13 to +14
extism call example/std_http.wasm _start --wasi --log-level info --allow-host "jsonplaceholder.typicode.com"
Copy link
Member Author

Choose a reason for hiding this comment

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

all go build-based wasm needs to be invoked via _start, as it is the only export present when using wasip1 GOOS.

@egonelbre
Copy link
Contributor

I should mention that in issue wasn't that the "strings" package was included per se... the main issue is that it's not possible to use the main package without having unicode tables included. i.e. if go list -deps includes unicode, then it's still a problem.

The only way to easily solve this is to move http things to a sub-package like github.com/extism/go-pdk/pdkhttp -- this would still allow easily using custom method names.

The other approach is to write a custom json encoder that doesn't rely on unicode tables.

Copy link
Contributor

@bhelx bhelx left a comment

Choose a reason for hiding this comment

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

Looks good to me. Are there any consequences to this?

@nilslice
Copy link
Member Author

nilslice commented Jan 5, 2024

@egonelbre - that's a good suggestion. will consider it!

@bhelx
Copy link
Contributor

bhelx commented Jan 17, 2024

Anything else we need to merge this?

@nilslice
Copy link
Member Author

I think it's good to go!

@nilslice nilslice merged commit 33b1e36 into main Jan 17, 2024
4 checks passed
@nilslice nilslice deleted the remove-strings branch January 17, 2024 19:58
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