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

Remove redundant width and height #28

Merged
merged 2 commits into from
Mar 28, 2024
Merged

Remove redundant width and height #28

merged 2 commits into from
Mar 28, 2024

Conversation

swsnr
Copy link
Collaborator

@swsnr swsnr commented Mar 28, 2024

These are already declared by Clutter.Actor, and our declarations are conflicting because they omit null.

Fixes the following compiler errors:

node_modules/.pnpm/@girs+gnome-shell@46.0.0-beta4/node_modules/@girs/gnome-shell/dist/ui/lightbox.d.ts:49:22 - error TS2320: Interface 'ConstructorProperties' cannot simultaneously extend types 'ConstructorProperties' and 'LightboxAdditionalParameters'.
  Named property 'height' of types 'ConstructorProperties' and 'LightboxAdditionalParameters' are not identical.

49     export interface ConstructorProperties extends St.Bin.ConstructorProperties, LightboxAdditionalParameters {
                        ~~~~~~~~~~~~~~~~~~~~~

node_modules/.pnpm/@girs+gnome-shell@46.0.0-beta4/node_modules/@girs/gnome-shell/dist/ui/lightbox.d.ts:49:22 - error TS2320: Interface 'ConstructorProperties' cannot simultaneously extend types 'ConstructorProperties' and 'LightboxAdditionalParameters'.
  Named property 'width' of types 'ConstructorProperties' and 'LightboxAdditionalParameters' are not identical.

49     export interface ConstructorProperties extends St.Bin.ConstructorProperties, LightboxAdditionalParameters {
                        ~~~~~~~~~~~~~~~~~~~~~


Found 2 errors.

These are already declared by Clutter.Actor, and our declarations are
conflicting because they omit null.
@swsnr
Copy link
Collaborator Author

swsnr commented Mar 28, 2024

/cc @Totto16

@swsnr
Copy link
Collaborator Author

swsnr commented Mar 28, 2024

@JumpLink I wonder why CI didn't catch these… libcheck flagged them in my extension, and VSCode immediately flagged them too when I opened the file 🤔

@Totto16
Copy link
Collaborator

Totto16 commented Mar 28, 2024

That might be a mess-up by me, true, it's strange, that they are not detected by the CI 🤔 (and by me locally they weren't either)
Only when using the submitted types to npm in the extension itself, the issue is shown, I'll investigate this, and merge this after I know all the details for this 👍🏼

@Totto16 Totto16 added the bug Something isn't working label Mar 28, 2024
@Totto16
Copy link
Collaborator

Totto16 commented Mar 28, 2024

@JumpLink I wonder why CI didn't catch these… libcheck flagged them in my extension, and VSCode immediately flagged them too when I opened the file 🤔

After some investigation, it never throws and error in any scenario for me locally, I did a full reset, and also than, it never failed.

I tested e.g.

export const u: St.Bin.ConstructorProperties

type U = typeof u.height

and U was number not number| null

But in @girs/clutter-14/clutter-14.d.ts::4968 it says indeed:

height?: number | null

validating in the CI / locally just uses tsc --noEmit and maybe the reason for not throwing an error is the lack of the strict
option in the tsconfig.json.

Anyway, it seems rather complicated, can you post your setup for the configuration, you get the error (I saw you using pnpm, I don't know if thats important or not... 🤔 (it shouldn't but who knows) )
So bundler, tsconfig.

And if you run yarn build (or just yarn validate if you already have built the types into ./packages/gnome-shell/dist/) locally, do you get an error or not 🤔

@Totto16
Copy link
Collaborator

Totto16 commented Mar 28, 2024

It really seems to be the tsconfig options, I set them to be:

{
	"compilerOptions": {
		"lib": ["ESNext"],
		"types": [],
		"target": "ESNext",
		"module": "ESNext",
		"moduleResolution": "Bundler",
		"strict": true,
		"noImplicitAny": true,
		"strictNullChecks": true,
		"noImplicitThis": true,
		"alwaysStrict": true,
		"strictPropertyInitialization": true,
		"allowSyntheticDefaultImports": true
	},
	"include": ["./dist/**/*.d.ts"],
	"files": ["./dist/index.d.ts"]
}

And than it fails 😂

Locally in the extension, I tested it on too, it has this config:

...
 "strict": true,
    "noImplicitAny": false,
    "strictNullChecks": true,
    "noImplicitThis": true,
    "alwaysStrict": true,
    "strictPropertyInitialization": false,
    "allowSyntheticDefaultImports": true,
...

strictNullChecks seems to be the root cause, even in my local extension it seems on, it doesn't get detected 🤔

@swsnr
Copy link
Collaborator Author

swsnr commented Mar 28, 2024

I didn't notice that this repo didn't set strictNullChecks and strict in its tsconfig. I think that should be corrected, i.e. these flags should be added. Ideally, the tsconfig in this repo would inherit from @tsconfig/strictest for recommended strict compiler settings, to make sure that these types are internally consistent.

As for why it doesn't fail for your extension, I presume you have libCheck disabled which will make typescript ignore all errors from library code.

@Totto16
Copy link
Collaborator

Totto16 commented Mar 28, 2024

I didn't notice that this repo didn't set strictNullChecks and strict in its tsconfig. I think that should be corrected, i.e. these flags should be added. Ideally, the tsconfig in this repo would inherit from tsconfig:strictest.

As for why it doesn't fail for your extension, I presume you have libCheck disabled which will make typescript ignore all errors from library code.

ah yes, libCheck 🤦🏼‍♂️

Copy link
Collaborator

@Totto16 Totto16 left a comment

Choose a reason for hiding this comment

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

Fixes the previously undiscovered bug 👍🏼

@Totto16 Totto16 merged commit 14a651f into gjsify:main Mar 28, 2024
1 check passed
@Totto16
Copy link
Collaborator

Totto16 commented Mar 28, 2024

@JumpLink I already bumped the version, you only need to publish the version to npm ❤️

@swsnr swsnr mentioned this pull request Mar 29, 2024
@swsnr swsnr deleted the remove-redundant-properties branch March 29, 2024 11:07
@swsnr
Copy link
Collaborator Author

swsnr commented Mar 29, 2024

@JumpLink Would you push a new version with this change?

@JumpLink
Copy link
Collaborator

@Totto16 , @swsnr I can also invite you to NPM, then you can do it yourself and don't have to wait for me

@JumpLink
Copy link
Collaborator

Is now published on NPM 👍

@Totto16
Copy link
Collaborator

Totto16 commented Mar 29, 2024

That would work too 👍🏼
My username on npm would be totto16

@swsnr
Copy link
Collaborator Author

swsnr commented Mar 30, 2024

@JumpLink I'm confused; I see a version on NPM but no release here? Did you forget to push a tag perchance?

@swsnr
Copy link
Collaborator Author

swsnr commented Mar 30, 2024

@JumpLink As for npm, I don't have an NPM account. I'll create one, but may take a few days as I'm away now.

@JumpLink
Copy link
Collaborator

JumpLink commented Mar 30, 2024

@swsnr Yes I forgot to draft a new release here on GitHub, did this now: https://github.com/gjsify/gnome-shell/releases/tag/46.0.0-beta5

@swsnr
Copy link
Collaborator Author

swsnr commented Apr 5, 2024

@JumpLink I finally got around to making an account on npm.js: https://www.npmjs.com/~swsnr

Sorry for the delay, easter's always pretty busy 😇

@JumpLink
Copy link
Collaborator

JumpLink commented Apr 5, 2024

@swsnr No problem, for me too :) I have sent you an invite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants