Skip to content
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

Tsserver throws error when class member names ends with $ #2338

Closed
ewal opened this issue Jun 4, 2022 · 20 comments
Closed

Tsserver throws error when class member names ends with $ #2338

ewal opened this issue Jun 4, 2022 · 20 comments

Comments

@ewal
Copy link

ewal commented Jun 4, 2022

Not really an Excalibur bug so I hope it's alright that I post this here.

The problem occurs when trying to extend the Actor class. The Actor extends the Entity class which have a couple of observables with names suffixed with the $ sign. When trying to add new members in my extended class, tsserver is throwing an error. Pics below.

I tracked down the problem to the version of TypeScript (4.6.3) that Excalibur uses. After I downloaded this project and updated TypeScript to the latest version, 4.7.3, the problem went away. Unfortunately I couldn't find a related ticket in TypeScript's Github repo and I've only seen this occur in Neovim.
The problem also goes away if I remove all dollar signs in the Entity class.

Steps to Reproduce

Open the Actor class in this project or extend the Actor class with (Neo)Vim and a configured tsserver plugin, eg. coc-tsserver.

Expected Result

With TypeScript 4.7.3
image

Actual Result

With TypeScript 4.6.3
image

Environment

  • operating system: osx
  • Excalibur versions: 0.26.0
@eonarheim
Copy link
Member

Hi @ewal thanks for the issue!

Oh wow, this is very interesting! It's wild that $ is messing with the tsserver/typescript, that should be a totally okay character to use in a variable name.

I have a colleague with a similar environment to yours that will be digging into the issue.

I found the related coc-tsserver issue you posted on (including here for posterity) neoclide/coc-tsserver#365

@eonarheim
Copy link
Member

I'm wondering if it's an odd interaction with that specific version of typescript's tsserver and the tsserver extension? At least in vscode pointing to that same version of typescript it seems to work okay.

There might not be anything we can directly do here other than upgrade typescript on main.

@ewal
Copy link
Author

ewal commented Jun 5, 2022

Hi, thanks for the reply! I've been fighting this issue for quite a while now and have had many different theories but it feels like I'm going in circles. Tbh, I'm not even sure that TypeScript is the cause anymore. It might as well be something with my environment. But, I do work in several other ts projects and I've only seen this particular problem once before after migrating a js-file to ts; code worked well in a brand new file with a different name but threw this error in the renamed file.
As I mentioned in the issue over at coc-tsserver, no errors are thrown when I manually configure the language server, but I still not getting any intellisense info in classes that extends Actor.

@ewal
Copy link
Author

ewal commented Jun 5, 2022

Edited: Deleted – too quick to comment...

@ewal
Copy link
Author

ewal commented Jun 6, 2022

The quest continues... So, updating TypeScript only solved part of my problem and I'm now pretty convinced that the issue lies elsewhere. I downloaded this repo and began commenting out everything in the Actor class and then re-added its members, piece by piece. I added a new test class in sandbox/src and found that when adding a setter to the Actor class, the error is thrown as shown in the gif. One step further but still no idea why this is happening :)

excalibur-set

Edit: This test was made with node 16.15.0 :/
Have to validate that this still occurs with node 14
Update: Same thing with node 14.17.0

@ewal
Copy link
Author

ewal commented Jun 6, 2022

Side note and definitely not related but, found a getter that can modify it's own return value in TransformComponen.ts, line 244. Not so dangerous here since the called method will set dirty to false, but bit of a risky pattern :)

  public get rotation(): number {
    if (this._dirty) {
      this._recalculate(); // <--- => this._rotation = this.matrix.getRotation();
    }
    return this._rotation;
  }

@eonarheim
Copy link
Member

@ewal Thanks for digging into this issue so deep, this is truly mysterious!

bit of a risky pattern :)

True true! This code will in theory go away soon :) I have some performance optimizations that hopefully makes the old transform implementation obsolete (in addition to being a lot simpler) https://github.com/excaliburjs/Excalibur/blob/perf/transform-matrix-refactor/src/engine/Math/transform.ts

@ewal
Copy link
Author

ewal commented Jun 8, 2022

I found the cause to the problem, ... 13+ days later 😬 I'll write a short summary soon. Was nearly going crazy there for a while.

@ewal
Copy link
Author

ewal commented Jun 18, 2022

Sorry for the delay, @eonarheim!

I decided to do some more testing. I still haven't figured out the source of the problem but at least I now know how to bypass it :) I have my suspects though, see further down.

TLDR;
The problem goes away if the consuming project is using TypeScript version <= 4.5.5

Using Excalibur in a new minimal project

  • Between tests all dependencies was removed and freshly reinstalled
  • The error message is show as soon as the IDE asks to show completions for a class that extends the eg. Actor class
  • It is NOT shown when asking for completions inside a class method or getter etc.
  • Not all classes throws this error, for example extending a Scene class works fine.
  • Only the consuming project needs to specify a lower TypeScript version
  • Excalibur was added as a local dependency and built with npm run clean && npm run build

Error message:
[coc.nvim] Error: Error processing request. Debug Failure. Did not expect GetAccessor t
o have an Identifier in its trivia
.... More

IDE: Neovim 0.7.0
coc-tsserver

(Tested two different computers with nvim and coc, Linux and Mac)

Minimal test project:
TypeScript:

  • 4.6.3
  • 4.5.5
  • 4.7.3
  • 4.7.4

Excalibur:

  • 0.26.0

Node:

  • 16.14.2
  • 16.15.1

Yarn:

  • 1.22.19

Excalibur source project:
TypeScript:

  • 4.6.3
  • 4.5.5
  • 4.7.3
  • 4.7.4

Node:

  • 16.14.2
  • 16.15.1

Npm:

  • 8.11.0

Test class

import * as ex from "excalibur";

export class Test extends ex.Actor {
// error is thrown as soon as you start typing here
}

I have no idea what the underlying problem is but I suspect that it is related to js-doc comments.
Example issue microsoft/TypeScript#48238
My idea is that TypeScript misinterpret some comment content where code examples are used... 🤷

I upgraded all dependencies in Excalibur and fiddled with the configurations for eslint, typescript, js-doc etc. which resultet in js-doc started to throw a lot of errors. Not of them saved but they all related to how the comments where formatted.

That's what I have so far :)

@ewal
Copy link
Author

ewal commented Jun 18, 2022

Btw, I have problems running the specs. The failing specs all relate to headless chrome and I can't find a solution. For example Chrome Headless 99.0.4844.0 (Linux x86_64) WebGL Adapter will not throw if accessed after GL creation FAILED.
Any idea?

@eonarheim
Copy link
Member

@ewal Interesting, it might mean that the WebGL context is failing to initialize in linux for some reason. Are you able to create a webgl2 context in your browser?

The WebGL accessor gets registered right after the context creation, if it failed it would explain the failing test
image

@ewal
Copy link
Author

ewal commented Jun 19, 2022

Replaced ChromeHeadless with ChromiumHeadless and got a bit further. Test still fails but for different reasons. On Linux now but gonna test in os x too.

@ewal
Copy link
Author

ewal commented Jun 19, 2022

Another thing – I can't install the packages in the main branch without either upgrading typedoc to the latest version or downgrading typescript to 4.5.5. That might be a hint that the problem comes from typedoc.
This is the error I get when trying to install modules after deleting the node_modules folder and package-lock.json:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: typedoc@0.22.11
npm ERR! Found: typescript@4.6.3
npm ERR! node_modules/typescript
npm ERR!   dev typescript@"4.6.3" from the root project
npm ERR!   peer typescript@">= 2.7" from fork-ts-checker-webpack-plugin@6.5.0
npm ERR!   node_modules/fork-ts-checker-webpack-plugin
npm ERR!     fork-ts-checker-webpack-plugin@"^6.0.4" from @storybook/builder-webpack5@6.4.17
npm ERR!     node_modules/@storybook/builder-webpack5
npm ERR!       dev @storybook/builder-webpack5@"6.4.17" from the root project
npm ERR!       2 more (@storybook/core, @storybook/core-server)
npm ERR!     fork-ts-checker-webpack-plugin@"^6.0.4" from @storybook/core-common@6.4.17
npm ERR!     node_modules/@storybook/core-common
npm ERR!       @storybook/core-common@"6.4.17" from @storybook/addon-controls@6.4.17
npm ERR!       node_modules/@storybook/addon-controls
npm ERR!         @storybook/addon-controls@"6.4.17" from @storybook/addon-essentials@6.4.17
npm ERR!         node_modules/@storybook/addon-essentials
npm ERR!       6 more (@storybook/builder-webpack4, ...)
npm ERR!   2 more (ts-loader, tsutils)
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer typescript@"4.0.x || 4.1.x || 4.2.x || 4.3.x || 4.4.x || 4.5.x" from typedoc@0.22.11
npm ERR! node_modules/typedoc
npm ERR!   dev typedoc@"0.22.11" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: typescript@4.5.5
npm ERR! node_modules/typescript
npm ERR!   peer typescript@"4.0.x || 4.1.x || 4.2.x || 4.3.x || 4.4.x || 4.5.x" from typedoc@0.22.11
npm ERR!   node_modules/typedoc
npm ERR!     dev typedoc@"0.22.11" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR! See /Users/adm/.npm/eresolve-report.txt for a full report.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/adm/.npm/_logs/2022-06-19T16_33_49_853Z-debug-0.log

@eonarheim
Copy link
Member

Thanks for the note! Looks like I had my user .npmrc misconfigured locally to legacy-peer-deps=true masking the issue for me!

I'll push and npm override for typedoc to the main repo, unfortunately upgrading typedoc is usually a bigger task. Typedoc seems to work for our purposes with it's TypeScript peer overriden temporarily.

@eonarheim
Copy link
Member

Also it's possible some of the text/font related tests may fail if the OS doesn't exactly render the fonts the same as our GH CI environment (ubuntu-latest) with Chrome. Text rendering is very operating system dependent unfortunately.

@ewal
Copy link
Author

ewal commented Jun 25, 2022

Should we close this issue? I'm using typescript 4.5.5 in my project which works fine and I seem to be alone experiencing this problem.

@eonarheim
Copy link
Member

Sure @ewal let me know if you run into new information and we'll look into it again 👍

@ewal
Copy link
Author

ewal commented Jul 16, 2022

@eonarheim, I really wanted a new feature in ts 4.7.4, still using 4.5.5, so I did a health check inside the ./src/engine folder by generating a new tsconfig template with npx tsc --init and did a build with tsc. TypeScript found 707 errors, but of course with just the default configuration for 4.7.4. Perhaps it's still a good idea to diff the changes with the latest. (Did a git blame and saw that the config haven't been updated in a while 👍 )

@eonarheim
Copy link
Member

@ewal Good observation, I've made an issue to evaluate the tsconfig and look into generating a more modern one.

#2420

Out of curiosity which new TS feature do you want to leverage?

@ewal
Copy link
Author

ewal commented Jul 18, 2022

Very nice 👍

The new feature is called moduleSuffixes. I thought it would help me solve a problem with a file import but actually I'd misunderstood its purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants