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

Fix TypeScript warnings #735

Merged
merged 2 commits into from
Mar 16, 2023
Merged

Conversation

bocharsky-bw
Copy link
Contributor

@bocharsky-bw bocharsky-bw commented Mar 16, 2023

Q A
Bug fix? yes
New feature? no
Tickets Fix #...
License MIT

If you run yarn build there are quite a few TS warnings. This happened because we originally didn't use TypeScript, so when we switched, a lot of "types" were missing.

I'm not sure how to fix the last one remaining here:

src/Notify/assets/src/controller.ts → src/Notify/assets/dist/controller.js...
(!) Plugin typescript: @rollup/plugin-typescript TS2769: No overload matches this call.
  Overload 1 of 3, '(type: "message", listener: (this: EventSource, ev: MessageEvent<any>) => any, options?: boolean | EventListenerOptions | undefined): void', gave the following error.
    Argument of type '(content: string | undefined) => void' is not assignable to parameter of type '(this: EventSource, ev: MessageEvent<any>) => any'.
      Types of parameters 'content' and 'ev' are incompatible.
        Type 'MessageEvent<any>' is not assignable to type 'string'.
  Overload 2 of 3, '(type: string, listener: (this: EventSource, event: MessageEvent<any>) => any, options?: boolean | EventListenerOptions | undefined): void', gave the following error.
    Argument of type '(content: string | undefined) => void' is not assignable to parameter of type '(this: EventSource, event: MessageEvent<any>) => any'.
      Types of parameters 'content' and 'event' are incompatible.
        Type 'MessageEvent<any>' is not assignable to type 'string'.
  Overload 3 of 3, '(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions | undefined): void', gave the following error.
    Argument of type '(content: string | undefined) => void' is not assignable to parameter of type 'EventListenerOrEventListenerObject'.
      Type '(content: string | undefined) => void' is not assignable to type 'EventListener'.
        Types of parameters 'content' and 'evt' are incompatible.
          Type 'Event' is not assignable to type 'string'.
src/Notify/assets/src/controller.ts: (61:56)

61             eventSource.removeEventListener('message', this._notify);
                                                          ~~~~~~~~~~~~
created src/Notify/assets/dist/controller.js in 926ms

If anyone has ideas how to fix - please, share :)

Depreciations like "(!) Plugin commonjs: The namedExports option from "@rollup/plugin-commonjs" is deprecated. Named exports are now handled automatically." were fixed in #734

@@ -2,22 +2,22 @@ import Component from './Component';
import { getElementAsTagText } from './dom_utils';

const ComponentRegistry = class {
private components = new WeakMap<HTMLElement, Component>();
#components = new WeakMap<HTMLElement, Component>();
Copy link
Member

@weaverryan weaverryan Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private should be ok - it’s a typescript thing (whereas # is pure js, so that’s also valid, but private should be ok in ts). But I see that this fixed a warning:

Property 'components' of exported class expression may not be private or protected.

Apparently the problem is that this is an anonymous class - microsoft/TypeScript#30355 (comment)

I'll comment the fix above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted in favor of class ComponentRegistry

!(fromEl instanceof HTMLElement || fromEl instanceof SVGElement) ||
!(toEl instanceof HTMLElement || toEl instanceof SVGElement)
!(fromEl instanceof SVGElement || fromEl instanceof HTMLElement) ||
!(toEl instanceof SVGElement || toEl instanceof HTMLElement)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this fix a ts warning? That would be wild - it looks logically identical

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did... but don't ask why :D My guess is that somehow the TS thinks it's pointless to check for SVGElement because it expects only HTMLElement. But somehow is fixed. Though I see you refactored that if without SVGElement check now :)

src/Notify/assets/src/controller.ts Show resolved Hide resolved
declare module '@swup/debug-plugin';
declare module '@swup/forms-plugin';
declare module '@swup/fade-theme';
declare module '@swup/slide-theme';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell more about this? We are not declaring any .d.ts files by hand at the moment. Are you fixing missing types from those swup libs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fairly speaking, I don't know for sure, but that's something that the warning suggested to do:

(!) Plugin typescript: @rollup/plugin-typescript TS7016: Could not find a declaration file for module 'swup'. '/Users/victor/www/symfony/ux/node_modules/swup/lib/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/swup` if it exists or add a new declaration (.d.ts) file containing `declare module 'swup';`

And since no @types/swup package, I did the 2nd suggestion. I just googled how to do it and it worked in this case

@weaverryan
Copy link
Member

Thanks Victor!

@weaverryan weaverryan merged commit f197685 into symfony:2.x Mar 16, 2023
@bocharsky-bw bocharsky-bw deleted the fix-ts-warnings branch March 17, 2023 08:46
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

Successfully merging this pull request may close these issues.

2 participants