-
Notifications
You must be signed in to change notification settings - Fork 108
Migrate to TypeScript 5.9 #1799
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?
Changes from all commits
d2dcdf1
7348764
a392268
0b77281
68e316a
ee7b2e3
27bba9d
6e252b6
3ab92ba
1630263
63320b0
bad081b
d8d6fa3
ab7273d
0710bcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
[*] | ||
end_of_line = lf | ||
end_of_line = lf |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,4 +27,4 @@ test-report.html | |
!.yarn/releases | ||
!.yarn/sdks | ||
!.yarn/versions | ||
yarn-error.log | ||
yarn-error.log |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
/src/alt-langs/ | ||
/src/py-slang/ | ||
/src/**/__tests__/**/__snapshots__ | ||
/src/**/__tests__/**/__snapshots__ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,7 +37,7 @@ | |
} | ||
)[] = [] | ||
) { | ||
// @ts-ignore | ||
// @ts-expect-error implicit any | ||
function _SourceHighlightRules(acequire, exports, _module) { | ||
'use strict' | ||
|
||
|
@@ -49,7 +49,8 @@ | |
const identifierRegex = '[a-zA-Z\\$_\u00a1-\uffff][a-zA-Z\\d\\$_\u00a1-\uffff]*' | ||
|
||
const chapter = variant === Variant.DEFAULT ? id.toString() : id.toString() + '_' + variant | ||
const builtin_lib = SourceDocumentation.builtins[chapter] | ||
const builtin_lib = | ||
SourceDocumentation.builtins[chapter as keyof typeof SourceDocumentation.builtins] | ||
|
||
function addFromBuiltinLibrary(meta: string) { | ||
if (builtin_lib === null) { | ||
|
@@ -120,9 +121,9 @@ | |
|
||
// Documentation for token types: | ||
// https://github.com/ajaxorg/ace/wiki/Creating-or-Extending-an-Edit-Mode#common-tokens | ||
// @ts-ignore | ||
Check warning on line 124 in src/editors/ace/modes/source.ts
|
||
const SourceHighlightRules = function (options) { | ||
// @ts-ignore | ||
Check warning on line 126 in src/editors/ace/modes/source.ts
|
||
const keywordMapper = this.createKeywordMapper( | ||
{ | ||
builtinconsts: getAllNames('const'), | ||
|
@@ -159,7 +160,7 @@ | |
'[4-7][0-7]?|' + //oct | ||
'.)' | ||
|
||
// @ts-ignore | ||
Check warning on line 163 in src/editors/ace/modes/source.ts
|
||
this.$rules = { | ||
no_regex: [ | ||
DocCommentHighlightRules.getStartRule('doc-start'), | ||
|
@@ -522,11 +523,11 @@ | |
} | ||
|
||
if (!options || !options.noES6) { | ||
// @ts-ignore | ||
Check warning on line 526 in src/editors/ace/modes/source.ts
|
||
this.$rules.no_regex.unshift( | ||
{ | ||
regex: '[{}]', | ||
// @ts-ignore | ||
Check warning on line 530 in src/editors/ace/modes/source.ts
|
||
onMatch: function (val, state, stack) { | ||
this.next = val == '{' ? this.nextState : '' | ||
if (val == '{' && stack.length) { | ||
|
@@ -567,13 +568,13 @@ | |
) | ||
|
||
if (!options || options.jsx != false) | ||
// @ts-ignore | ||
Check warning on line 571 in src/editors/ace/modes/source.ts
|
||
JSX.call(this) | ||
|
||
// Adding of highlight rules for Source Typed | ||
// Code referenced from https://github.com/ajaxorg/ace-builds/blob/master/src/mode-typescript.js | ||
if (variant === Variant.TYPED) { | ||
// @ts-ignore | ||
Check warning on line 577 in src/editors/ace/modes/source.ts
|
||
this.$rules.no_regex.unshift( | ||
{ | ||
token: ['storage.type', 'text', 'entity.name.function.ts'], | ||
|
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.
Shall we reraise this as an issue? I know that in #1516 it was agreed that the component was useful, but with the problems we are having with it (increased complexity, here we see strange issues with types) we may want to reimplement it (probably only viable as a student project if it's breaking?) or reevaluate it again
Uh oh!
There was an error while loading. Please reload this page.
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.
Normally, I try to fix type issues as I go (even if it means weird typecasts etc.) but this is beyond me; either it's some super old TS logic that predates my knowledge of the "modern" 4.x and 5.x versions or it's just (ab)use of TS that was somehow never caught.
Without looking at the code and figuring out how this all works I had to resort to just putting an
any
there.I suspect the original type definitions may have not been entirely correct, but since TS used to implicitly give an
any
for these kinds of invalid accesses (and they are accessed in a way consistent and expected of the actual contract/behaviour) then it all just magically seems to work.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'm ok with removing the feature for now. Re-implementing it will lead to a cleaner implementation.
The project is well-documented in a report and can be revived from there.
I don't think the system will be missed very much. It carries a bit of infrastructure with a server side, sentry support etc that was needed for carrying out the project.
So yea, please go ahead and remove it.