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

Stricter TS checks in Vaadin 23.3.0 #15485

Closed
javier-godoy opened this issue Dec 14, 2022 · 10 comments · Fixed by #15700
Closed

Stricter TS checks in Vaadin 23.3.0 #15485

javier-godoy opened this issue Dec 14, 2022 · 10 comments · Fixed by #15700

Comments

@javier-godoy
Copy link
Contributor

Description of the bug

Vaadin 23.3.0 seems to introduce stricter TS checks by default.

I had to configure the following in tsconfig.json in order to run applications that were working fine with Vaadin 23.2:

"noImplicitAny": false,
"noImplicitThis": false,
"strictPropertyInitialization": false

(noImplicitAny and noImplicitThis were already true with Vaadin 23.2, but now they flag additional errors.)

See some sample error logs in FlowingCode/FontAwesomeIronIconset#56

The new stricter defaults are problematic, since they also affect third-party components (addons), which may rely on loose checks.

Expected behavior

Since it's not documented as a breaking change, I would expect no additional TS errors when upgrading from Vaadin 23.2 to Vaadin 23.3.0.

Minimal reproducible example

See https://github.com/FlowingCode/FontAwesomeIronIconset/tree/font-awesome-iron-iconset-4.2.0 (fixed in FlowingCode/FontAwesomeIronIconset#58)
and https://github.com/FlowingCode/XTermConsoleAddon/commits/xterm-console-2.1.0 (fixed in FlowingCode/XTermConsoleAddon#66)

The application runs with no issues up to vaadin 23.2 (mvn jetty:run), but it fails with TS errors with vaadin 23.3 and 24.

Versions

  • Vaadin / Flow version: 23.3.0, 24.0.0.alpha5
  • Java version: 11, 17
  • OS version: Windows 10
@Artur-
Copy link
Member

Artur- commented Dec 14, 2022

So what has changed as the options are still the same? Is it because of upgrading Typescript from 4.7 to 4.9?

mcollovati added a commit that referenced this issue Dec 14, 2022
Principal name usually refers to a user identifier that can be
used to retrieve additional details.

Fixes #15485
@javier-godoy
Copy link
Contributor Author

Is it because of upgrading Typescript from 4.7 to 4.9?

It seems so, although I cannot fully parse the TypeScript release notes.

Spelling out the details for one of the example cases I mentiones above:

  1. Checkout branch flow-15485 from FlowingCode/FontAwesomeIronIconset (pom has vaadin 23.2.10)

git clone -b flow-15485 git@github.com:FlowingCode/FontAwesomeIronIconset.git

  1. Run the demo application mvn jetty:run, open http://127.0.0.1:8080/ --> the application starts, no errors 🙂

  2. mvn vaadin:dance

  3. Edit pom and set 23.3.0

  4. Delete tsconfig.json so that it's recreated with defaults.

  5. mvn jetty:run, open http://127.0.0.1:8080/ --> the application starts, there are some errors 🙁

Property has no initializer and is not definitely assigned in the constructor. 

https://github.com/FlowingCode/FontAwesomeIronIconset/blob/flow-15485/src/test/resources/META-INF/resources/frontend/fc-font-awesome-gallery.ts#L9-L19

Variable implicitly has type 'any[]' in some locations where its type cannot be determined.
Variable implicitly has an 'any[]' type.

https://github.com/FlowingCode/FontAwesomeIronIconset/blob/flow-15485/src/test/resources/META-INF/resources/frontend/fc-font-awesome-gallery.ts#L67-L71

Object is possibly 'null'.

https://github.com/FlowingCode/FontAwesomeIronIconset/blob/9a7597b9092ffb382577fba1ac04a7e8662a08a5/src/test/resources/META-INF/resources/frontend/fc-font-awesome-gallery.ts#L79

  1. rm -rf frontend/generated

  2. Edit tsconfig.json and set "noImplicitAny": false and "strictPropertyInitialization": false

  3. mvn jetty:run, open http://127.0.0.1:8080/ --> the application starts, there are less errors 🤨

Type 'string[]' is not assignable to type 'never[]'. Type 'string' is not assignable to type 'never'.
Object is possibly 'null'.
  1. Edit tsconfig.json and set "strictNullChecks": false (https://stackoverflow.com/a/57563877/1297272)

  2. Run the demo application mvn jetty:run, open http://127.0.0.1:8080/ --> the application starts, no errors 🙂

@Artur-
Copy link
Member

Artur- commented Dec 14, 2022

It just hit me what the change is: the files from addons are validated as part of the build as they are placed inside frontend/generated/jar-resources. That folder should be excluded as you cannot change the code in the addons

@probert94
Copy link

I am facing the same issue in a multi module project I am working on.
During the build I get a lot of Parameter implicitly has an 'any' type. errors.
The files with the errors always come from "frontend/generated/jar-resources/src".
The errors are actually true but they are not shown on the real files ("META-INF/resources/frontend/src") neither in VS Code nor during the build.
When I (accidentally) open one of the generated files in VS Code, the error is shown there too, so it seems like a different tsconfig.json is used for the "real" files and the generated files.

Artur- added a commit that referenced this issue Jan 17, 2023
The TS files that come from JAR files are not under control of the project developer so enforcing project TS rules on them does not make sense. This is similar to "skipLibCheck" used for TS files that are in node_modules.

Fixes #15485
@Artur-
Copy link
Member

Artur- commented Jan 17, 2023

@probert94 Try adding

 "exclude": [
    "frontend/generated/jar-resources/**"
  ]

to your tsconfig.json. This is also what is done in #15700 for future versions

@Artur-
Copy link
Member

Artur- commented Jan 31, 2023

It seems like "exclude" only works if you do not reference the files from any code... which kind of is not what ever happens

Artur- added a commit that referenced this issue Jan 31, 2023
The TS files that come from JAR files are not under control of the project developer so enforcing project TS rules on them does not make sense. This is similar to "skipLibCheck" used for TS files that are in node_modules.

Fixes #15485
Artur- added a commit that referenced this issue Jan 31, 2023
The TS files that come from JAR files are not under control of the project developer so enforcing project TS rules on them does not make sense. This is similar to "skipLibCheck" used for TS files that are in node_modules.

Fixes #15485
@Artur-
Copy link
Member

Artur- commented Jan 31, 2023

Now I am confused because #15700 does seem to work..

@javier-godoy
Copy link
Contributor Author

Just noting that I applied the workaround in a couple of projects and addons, and it works fine.

mshabarov pushed a commit that referenced this issue Feb 2, 2023
The TS files that come from JAR files are not under control of the project developer so enforcing project TS rules on them does not make sense. This is similar to "skipLibCheck" used for TS files that are in node_modules.

Fixes #15485
vaadin-bot pushed a commit that referenced this issue Feb 2, 2023
The TS files that come from JAR files are not under control of the project developer so enforcing project TS rules on them does not make sense. This is similar to "skipLibCheck" used for TS files that are in node_modules.

Fixes #15485
Artur- added a commit that referenced this issue Feb 2, 2023
The TS files that come from JAR files are not under control of the project developer so enforcing project TS rules on them does not make sense. This is similar to "skipLibCheck" used for TS files that are in node_modules.

Fixes #15485
vaadin-bot added a commit that referenced this issue Feb 2, 2023
… (CP: 23.3) (#15803)

* fix: Do not perform type checking for TS files from jar files (#15700)

The TS files that come from JAR files are not under control of the project developer so enforcing project TS rules on them does not make sense. This is similar to "skipLibCheck" used for TS files that are in node_modules.

Fixes #15485

* add import

---------

Co-authored-by: Artur <artur@vaadin.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.0.0.alpha10 and is also targeting the upcoming stable 24.0.0 version.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.3.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment