search

JamesHenry / typescript-estree

:sparkles: A parser that converts TypeScript source code into an ESTree-compatible form
https://jameshenry.blog
Other
84 stars 13 forks source link

WIP: refactor converting #80

Closed armano2 closed 5 years ago

armano2 commented 5 years ago

This PR is still WIP

Goals:

JamesHenry commented 5 years ago

I am currently at the tail end of a vacation with my family. It's very important to take some time away from coding and reconnect with the important people in my life, and it was long overdue for me this year.

That does not mean your PRs are not important or appreciated, but it does mean that there has been a (small) delay in reviewing them.

Regarding this one, I feel I was very understanding, but also clear in https://github.com/JamesHenry/typescript-estree/pull/52#issuecomment-449486413

I want to show empathy - we haven't had the benefit of meeting face to face and we have different approaches which is a great thing and can lead us to better outcomes - but I am really struggling to understand how you feel it can be a productive way to communicate by just opening an unfinished, major refactoring of a library, without any context or discussion first. Particularly after I called out it twice in that recent comment that issues would be best for such things.

I currently have no background as to why you are making these changes. It is very inefficient for the burden to be on me as the maintainer to attempt to infer the background myself from what you have already done.

Your bullet points don't really tell me anything:

limit initialization of functions

Why is this an issue? What benchmarking have you done that shows an existing issue and a clear improvement with this PR?

reduce stack-trace

Why is this an issue?

improve type checking

Great goal to have, but does it need to come in the form of a total refactor?

fix issues while running multiple conversions in parallel

What issues? What scenarios are you using parallel conversion in?

Opening an issue to discuss first (i.e. giving me a little time to reply first), would have allowed us to explore the following points:

Again, I do not want to dissuade you from contributing. It is amazing how much work you have done over the holidays when most people are taking it easy, but we have to be able to work productively together on this.

Please let me know if there is anything else I can do on my side to facilitate this and I will work hard to imrpove it (Naturally I mean over and above getting back to you on your existing PRs and issues, and please note this will still take a few days, I am travelling from Abu Dhabi to Toronto tomorrow, a 14.5 hour flight, across 9 timezones).

armano2 commented 5 years ago

I'm a maintainer of https://github.com/vuejs/eslint-plugin-vue and we are slowly planning to add support for TS to it. but when i started trying to make it work i had many issues with typescript-eslint-parser. Then i looked into those projects and i noticed that they are pretty dead... thats why i decided to help with this. if you don't like this idea, its ok.

i don't like explaining stuff, it's easier to write it, and i don't really care if they are not going to be accepted.

i'm kinda new to typescript internals and this is great way to learn about it 🤖

JamesHenry commented 5 years ago

That's great, and I do really appreciate the help! It was me working basically single-handed in my spare time for a couple of years, so believe me I do really appreciate it :)

Please could you help me understand what the current issues are and how this helps solve them? Definitely open to changes and I appreciate the preference of writing first, but without at least some background it makes it difficult to review.

armano2 commented 5 years ago

this PR whas splitting converter logic to separated functions, and giving us opportunity to easily fix https://github.com/JamesHenry/typescript-estree/blob/master/src/convert.ts#L2096

it's additionally was making code much cleaner to read/review.

right now when we execute recursive function convert we have to reinitialize over and over all helper functions from within.

additionally we have pass configs ast tree (pointer) and so on....

right now there is also no easy way to add proper typing for result using ESTreeNode is not perfect and its not telling to outsider much.

it's not possible to find out what you will get or what you should get as resulted AST.

parallel builds properties like esTreeNodeToTSNodeMap, tsNodeToESTreeNodeMap are stored outside function https://bytearcher.com/articles/parallel-vs-concurrent/ https://v8.dev/blog/concurrent-marking

armano2 commented 5 years ago

@JamesHenry just for this i did small performance test: https://jsperf.com/cost-function-vs-class

this is really simple test, but represents well difference in time