-
Notifications
You must be signed in to change notification settings - Fork 75
Create a JS LST doc #434
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
base: master
Are you sure you want to change the base?
Create a JS LST doc #434
Conversation
To explain the JS LST types and how they work with J types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
One thing I noticed that not all JS LST elements are mentioned. Not sure if this doc aims at being an exhaustive reference. If so, they should be added. If it's meant just to indicate the concepts of two "namespaces" like J and JS - then I suggest to add some disclaimer sentence at the top like "Some of the most important JS types are:" or similar.
And if the latter - then some of the JS elements mentioned don't seem so crucial to me - e.g. all these called *Type*.
| **Code example:** | ||
|
|
||
| ```typescript | ||
| 1 + 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good example of a JS Binary. 1+2 is modelled as J.Binary.
JS.Binary contains these binary operators which don't exist in Java. For instance ===.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting - will update, thanks.
| A [JS.Import](https://github.com/openrewrite/rewrite/blob/v8.65.0/rewrite-javascript/src/main/java/org/openrewrite/javascript/tree/JS.java#L1202-L1304) represents an ES6 import statement. It contains an [ImportClause](https://github.com/openrewrite/rewrite/blob/v8.65.0/rewrite-javascript/src/main/java/org/openrewrite/javascript/tree/JS.java#L1310-L1381) (what is being imported) and the module path as a Literal. | ||
|
|
||
| **Code examples:** | ||
|
|
||
| ```typescript | ||
| import { Helper } from './utils'; | ||
| import type { Config } from './types'; | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we want to add it here, but maybe worth mentioning that another kind of import in TS is the one using require(). And these are not modelled as JS.Import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair! What is require() modeled as?
| ### ConditionalType | ||
|
|
||
| A [JS.ConditionalType](https://github.com/openrewrite/rewrite/blob/v8.65.0/rewrite-javascript/src/main/java/org/openrewrite/javascript/tree/JS.java#L469-L543) represents TypeScript conditional types using the `extends` keyword and ternary-like syntax. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if it's worth extracting all the type specific LST elements to a separate subsection?
IMHO they are somewhat special and might not be obvious at the first glance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust your judgement better than my own here. I know very little about working with LSTs and what's important to people - this whole doc was largely just me trying to piece together things from a hacked together LST (and going back and forth with Claude trying to step through code and figure out what's going on).
Could you expand on what you'd like to see in this section and what makes them special? What other information can we add to help?
| **Code examples:** | ||
|
|
||
| ```typescript | ||
| add(item: T): void { | ||
| this.items.push(item); | ||
| } | ||
|
|
||
| async fetch(url: string): Promise<T[]> { | ||
| const response = await fetch(url); | ||
| return response.json(); | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO worth adding an example for function as an expression, which is not directly available in Java. And should be a nice example of the "hybridness". We still model such a function as J.MethodDeclaration.
Something like:
const x = function() { return 136; }
or something a bit more interesting.
| ### MethodInvocation | ||
|
|
||
| A [J.MethodInvocation](https://github.com/openrewrite/rewrite/blob/v8.65.0/rewrite-java/src/main/java/org/openrewrite/java/tree/J.java#L4177-L4347) represents function and method calls. The structure is similar to Java, with a select expression (for method calls) and arguments. | ||
|
|
||
| **Code examples:** | ||
|
|
||
| ```typescript | ||
| this.items.push(item) | ||
| fetch(url) | ||
| response.json() | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section reminded me of JS.FunctionCall, which I see no mention of in the document.
Is it on purpose? If so, feel free to ignore this comment.
Otherwise - it's worth mentioning when things end up as JS.FunctionCall instead of J.MethodInvocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not purposeful. Do you think we should add it? Can you explain more on when things end up as a function call vs. a method invocation?
|
Thanks for the feedback @greg-at-moderne -- my intention wasn't to provide all LST types but, rather, to just pick the most important ones. I did my best guess at it - but I don't really work with any of this stuff so there's definitely bound to be mistakes 😅 Is every LST type that's really important included in here? Is the main issue that we have 3 Type* LSTs defined that aren't really important? Either way - I'll make sure to update the doc with a clearer intro that only some of the types are included. |
To explain the JS LST types and how they work with J types.