-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Introduce JSDoc-based Type System with checkJs
flag
#5318
Conversation
# Conflicts: # src/selection.js
# Conflicts: # src/autocomplete.js
# Conflicts: # package.json # src/lib/app_config.js # src/lib/lang.js
Nice work!! I agree on the |
src/keyboard/textinput.js
Outdated
|
||
text.style.opacity = "0"; | ||
parentNode.insertBefore(text, parentNode.firstChild); | ||
|
||
var copied = false; | ||
var pasted = false; | ||
/** | ||
* | ||
* @type {false | {[key: string]: any}}} |
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.
Any opinion about this style vs Record<string, any>
?
return types.indexOf(token.type || token) > -1; | ||
}; | ||
|
||
CstyleBehaviour.recordAutoInsert = function(editor, session, bracket) { | ||
CstyleBehaviour["recordAutoInsert"] = function(editor, session, bracket) { |
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 would rather aim to make it work as it is, without trying to please TS language server too much by rewriting everything into ["..."]. You probably just need to define a proper typedef.
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.
Generally I agree with you, but lets leave it for further improvements (especially whole mode/*
part)
Very nice work! 👍 I just happen to develop a tool right now which parses JavaScript projects with JSDoc via Babel and then adding automatic type assertions at runtime. I use it so far only for the PlayCanvas game engine and it helped me to squeeze out a bunch of bugs already and I'm looking to support more projects. However, it's under heavy construction right now because I prepare for an official release to make it work for all kinds of different projects: https://github.com/kungfooman/RuntimeTypeInspector.js (You can watch the first video to see how it works) Once I have more time, I will look into the integration here. Very happy to see better JSDoc across all kinds of projects 💪 💯 🥇 |
@nightwing, @whazor, @akoreman, @InspiredGuy I'm hoping to gather thoughts and suggestions on a few decisions we're considering regarding new type system, because it would influence everyone's experience:
/**
* @typedef {import("../ace").Ace.Point} Point
*/ For each type sourced from |
Typedef type aliasing has pretty much two issues:
If you don't care about two mouse clicks and just wanna put a |
Hey, nice work!! 🚀 I think I am kind of lost though with what we want to achieve with the generated types:
Do you know of any other popular open source project relying on JSDoc? |
It could provide better intellisense for internal development purposes, but right now I also don't see a lot of benefits on generating types except the case when it would help to convert project to plain typescript.
Personally, I anticipate consumers won't have to change their usage much, which is why I retained the type aliases in
I don't know of many projects using this approach, as it's often a temporary solution before migrating to full TypeScript. However, some examples include: P.S.: many cases of uses mixins (EventEmitter and OptionsProvider mostly) in aces classes make us to provide declare module "ace-code/src/editor" {
export const Editor: {
new(renderer: import("./src/virtual_renderer").IVirtualRenderer, session?: import("./src/edit_session").IEditSession, options?: Object): import("./src/editor").IEditor;
};
} or import {Ace} from "../ace";
import {IEditSession} from "../src/edit_session";
import {IVirtualRenderer} from "../src/virtual_renderer";
export const Editor: {
new(renderer: IVirtualRenderer, session?: IEditSession, options?: Object): Ace.Editor;
}; for any public ace module (actually everything in |
Also, according to different structure of ace-code and ace-builds it's a bit challenging to solve everything and not break something in process :) |
@mkslanc Looks like you got quite far in the process indeed! Can we keep it without Step 3 though? That way we improve the situation right now and merge the majority of this PR but lower the risk of introducing consumer changes. |
@marinsokol5 I think it shouldn't be hard to separate those parts - I will leave only JSDoc's changes + tsconfig in that PR and create separate PR with generator. But maybe we will need to separate "typing" in "package.json" for internal and external (published) usage. I need to do few tests to be sure of that |
# Conflicts: # ace.d.ts # src/autocomplete.js
Please test it thoroughly. I don't want to introduce any regressions to the codebase. :) P.S.: from my side I tested it in different packages, where we used |
Is it possible to keep |
@marinsokol5 It's possible, but I will need to change all import("../ace").Ace. to import("../ace-internal").Ace. If this is acceptable, I will make changes |
@mkslanc Is it possible to not have ace-internal.d.ts at this moment in time, just ace.d.ts? |
@marinsokol5 Unfortunately, no. Types in JSDoc's are highly dependent on |
@marinsokol5 Also we could set "checkJs" to false, but it would nullify all advantages of type-checking |
# Conflicts: # ace.d.ts # src/autocomplete.js
@mkslanc sadly we need to generate the .d.ts files otherwise this example is failing ajaxorg/ace-samples@master...ts, and we may even need the generated file to pass strict check which is specified in the failing test. but not sure about that. |
# Conflicts: # ace.d.ts # src/autocomplete.js
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5318 +/- ##
==========================================
+ Coverage 87.58% 87.59% +0.01%
==========================================
Files 583 583
Lines 46369 46389 +20
Branches 7016 7018 +2
==========================================
+ Hits 40610 40636 +26
+ Misses 5759 5753 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@nightwing Separate internal and external declaration files, should make it for now. I think generator should be separate PR, especially considering the fact that we need to run it in strict mode |
src/selection.js
Outdated
@@ -77,7 +69,7 @@ class Selection { | |||
|
|||
/** | |||
* Returns an object containing the `row` and `column` current position of the cursor. | |||
* @returns {Object} | |||
* @returns {import("../ace-internal").Ace.Point} |
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.
Could you convert this to a typedef, and do the same for other instances where same import() is used several times in a file?
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.
Yes, sure, but users would need to click twice to see the real type declaration. If this is ok, I would process with changes across codebase
79a25f8
to
c6398d6
Compare
c6398d6
to
07b0241
Compare
Issue #, if available:
Description of changes:
This PR introduces a comprehensive type system for our plain JavaScript project, leveraging the power of JSDocs and TypeScript's
checkJs
feature.Details:
tsconfig.json
to enablecheckJs
, allowing TypeScript to check our plain JavaScript files for type errors.Benefits:
P.S.: I left //@ts-expect-error with TODO in different places in codebase, where I think some bugs could occur
P.P.S.: not whole classes/ functions covered with types right now, somewhere I left
any
types, because it already become quite big change. Could be addressed in separate PR.ace.d.ts
generator for ace-code and ace-buildsBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.