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

typescript profile parser #124

Merged
merged 13 commits into from
Feb 2, 2024
Merged

Conversation

TheEdward162
Copy link
Contributor

tracking work on typescript profile parser

@TheEdward162 TheEdward162 force-pushed the feat/typescript_profile_parser branch from 87f787a to fe717f4 Compare December 7, 2023 20:15
* rename Comlink language parser to make room for TypeScript parser integration
@TheEdward162 TheEdward162 force-pushed the feat/typescript_profile_parser branch from fe717f4 to 78eb7a3 Compare January 4, 2024 19:17
@TheEdward162 TheEdward162 force-pushed the feat/typescript_profile_parser branch from 78eb7a3 to ac6fb58 Compare January 5, 2024 23:29
* create comlink_wasm package
* create wasm_abi package by extracting the abi part from host_to_core_std, making it reusable
* to allow lower-level sharing of common/lib
* copy from language-server repo
* change to use comlink wasm
* We mainly use two forms: `UseCase` as a camel cased variant and `usecase` as snake case variant. This is already inconsistent but changing the snake case variant would be a big breaking change.
* This commit changes usages of `Usecase` into `UseCase`
@TheEdward162 TheEdward162 marked this pull request as ready for review January 11, 2024 21:47
@TheEdward162
Copy link
Contributor Author

TheEdward162 commented Jan 11, 2024

This PR got a little fat because I included some OCD changes, like consolidating names/variables named Usecase to UseCase. I think going commit-by-commit might help with review.

Either way, it can also be reviewed by trying it out with the vscode extension as I posted in Slack.

Copy link
Member

@freaz freaz left a comment

Choose a reason for hiding this comment

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

Well lets be honest, I tried to read it and understand, but there are places I am not sure. Since it seems to work, I don't want to block this PR anymore.

Searching "things" in TypesScript AST is 🤯, but most likely, because I didn't get much used to AST biome parser produces.

few nitpicks:

  • packages/comlink_language_server/src/parser/index.ts seems to mix tabs and spaces
  • found missing ;

praises:

  • I like we have now wasm_abi as separate crate
  • Comlink tooling in one crate

core/comlink/src/lib.rs Outdated Show resolved Hide resolved
packages/comlink_language_server/src/parser/index.ts Outdated Show resolved Hide resolved
packages/comlink_language_server/src/parser/index.ts Outdated Show resolved Hide resolved
packages/javascript_common/src/app.ts Outdated Show resolved Hide resolved
@@ -21,6 +23,7 @@ function strace<A extends unknown[], R>(name: string, fn: Fn<A, R>, asyncify: As
return result;
}
}
// @ts-ignore ignore unused function
Copy link
Member

Choose a reason for hiding this comment

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

why is it kept?

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 left these thinking it can come in handy in two cases:

  1. as a documentation of how to work with the ABI from TypeScript
  2. in case you are debugging your code by monkey-patching it, then these can come in handy since they will be in the dist already. However I did not quite realize that bundlers will probably notice these are unused and remove them.

I think we can safely remove them.

@@ -37,6 +40,7 @@ function join_abi_result(lower: number, upper: number): number {

return l | u;
}
// @ts-ignore ignore unused funtion
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Some((example, spans))
}

fn parse_example_element_value(&mut self, expr: AnyJsExpression) -> JsonValue {
Copy link
Member

Choose a reason for hiding this comment

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

I don't even remember we allowed to have expressions in example values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even literals are wrapped in expression node when in expression position. Since examples are of the form:

const examples: UseCaseName['examples'] = [
  { input: 1, result: 2 }
]

Then all of [], {}, 1 and 2 are something like Expression(Literal) nodes in the tree.

@TheEdward162 TheEdward162 merged commit 1d6df07 into main Feb 2, 2024
5 checks passed
@TheEdward162 TheEdward162 deleted the feat/typescript_profile_parser branch February 2, 2024 15:56
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.

2 participants