kaleidawave / ezno

A JavaScript compiler and TypeScript checker written in Rust with a focus on static analysis and runtime performance

https://kaleidawave.github.io/posts/introducing-ezno/

MIT License

2.3k stars 42 forks source link

[Feat] Adding `export type { ... } from '...'` support #84

Closed JulesGuesnon closed 7 months ago

JulesGuesnon commented 7 months ago

Description

Hey! Following my issue #83 here is a PR to support this feature!

I would need 2 advices:

I couldn't return UnexpectedToken error as I can't clone a Token, so I'm not sure how I can return it
There's a println!("Not supposed to happen"); in the code, what would the intended error in that case?

kaleidawave commented 7 months ago

Awesome! I wondered whether export type was syntax TypeScript supported, looks like it is hidden in there.

Great change (well done for also adding the change on the synthesis side + in the test file) and passing all the tests (ignore the fuzzing ones, they're broken).

Some suggestions:

Maybe rather than a new variant of ExportDeclaration, instead Exportable::ImportParts could have a Option<Keyword<tsx_keywords::Type>> field like ImportDeclaration has. That would fix needing a println!("Not supposed to happen"); branch :)
Instead of checking what comes after the } bracket, as importing parts is the only case for parsing you can just go ahead and run crate::parse_bracketed::<ExportPart> all the time. Then the line under // Know this is 'from' from above should become reader.expect_next(TSXToken(TSXKeyword::From)) to make sure from is expected.

Let me know if you want any more hints :). After that it should be good for merging 👍

JulesGuesnon commented 7 months ago

Hey @kaleidawave !

Yeah I agree, it's not well documented!

I updated according to your reviews feel free to tell me if there's any other required change!

Btw, I noticed clippy was raising a lot of warnings, would you mind if I do another PR to fix all of this?

kaleidawave commented 7 months ago

Merged 👍

Yes I currently have clippy just printing warnings. If you have the time and could go through and tidy some of the code up that would be great. However if you are touching anything under checker, I would wait until #78 if merged as it contains a lot of changes

JulesGuesnon commented 7 months ago

Thanks for the merge! There's definitely a lot of changes in the PR you mentioned, I'll wait for it to be merged I think

kaleidawave commented 7 months ago

Hey @JulesGuesnon, #78 has been merged, think I will take a little break from the checker for now 😅. Feel free to take a look at improving the code based on the clippy output. I think there a lot of stuff that was just quickly implemented to get it out.

I checked it the other day and there looks like there is a lot. Hopefully some can be cleaned up which can improve the codebase and make future features easier.