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

Autodoc: Javascript port of zig lexer. #16306

Merged
merged 6 commits into from
Jul 3, 2023
Merged

Conversation

Myvar
Copy link
Contributor

@Myvar Myvar commented Jul 2, 2023

No description provided.

@Myvar Myvar requested a review from kristoff-it as a code owner July 2, 2023 18:32
@andrewrk
Copy link
Member

andrewrk commented Jul 2, 2023

Why is it written in javascript instead of compiling our existing tokenizer to wasm?

This introduces quite a bit of redundant code to maintain, so I think there should be a clear justification.

@kristoff-it
Copy link
Member

Why is it written in javascript instead of compiling our existing tokenizer to wasm?

This introduces quite a bit of redundant code to maintain, so I think there should be a clear justification.

We discussed yesterday this in a call and decided to avoid bumping up the minimum requirements for using autodoc until we're confident we want to opt into WASM with more than just a tokenizer. Using a WASM tokenizer also brings in the usual string-related issues where the bytes have to exist both in wasm as shared memory and as a separate copy accessible to the JS engine / browser, increasing memory consumption & slowing down the rendering process.

@Myvar
Copy link
Contributor Author

Myvar commented Jul 3, 2023

@kristoff-it

1) the function generate_html_for_src is implemented in a very naive way, it's just a demo for using the lexer we should build a better version for actual Autodoc

  1. testing this is complicated, it does not have to be complicated and we can certainly remove/fix the complexity but that is out of scope for this pr maybe in the future, for now just hit me up on Discord and il show you how testing it works

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.

None yet

3 participants