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

feat: fix set issues for TypeScript 5.5+ #421

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chrisvdp
Copy link

#417

This fixes the issues introduces in Typescript 5.5 by updating the types used in Map and Set and implementing the new properties on Set.

This is a breaking change and would require users to be on Typescript 5.5+.

},
"skipLibCheck": true
"skipLibCheck": true,
"lib": [
Copy link
Author

Choose a reason for hiding this comment

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

not sure if this should be added here, but it was the only way to get the errors to show properly when developing and adding tests.

@SergeAstapov SergeAstapov added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Oct 23, 2024
package.json Outdated Show resolved Hide resolved
addon/package.json Outdated Show resolved Hide resolved
test-app/package.json Outdated Show resolved Hide resolved
@SergeAstapov
Copy link
Collaborator

Thank you @chrisvdp for contribution! Could you please fix lint errors?

@chrisvdp
Copy link
Author

@SergeAstapov updated with your feedback

@chrisvdp
Copy link
Author

@SergeAstapov I seem to have run into an issue with typescript-eslint and TS version support.

It seems that 5.6 was added in version 8.10, but that requires an overhaul to eslint configs in general. typescript-eslint/typescript-eslint#9653

5.5 support was added in v7.14.1.

Downgrading the version of TS here to 5.5 to avoid the eslint upgrade.

@chrisvdp chrisvdp force-pushed the patch-1 branch 2 times, most recently from 757c17a to 4c25c85 Compare October 30, 2024 16:39
@@ -55,6 +55,7 @@ function convertToInt(prop: number | string | symbol): number | null {
return num % 1 === 0 ? num : null;
}

// eslint-disable-next-line @typescript-eslint/no-unsafe-declaration-merging
Copy link
Author

Choose a reason for hiding this comment

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

"@typescript-eslint/eslint-plugin": "^5.40.0",
"@typescript-eslint/parser": "^5.26.0",
"eslint": "^8.25.0",
"@typescript-eslint/eslint-plugin": "^7.14.1",
Copy link
Author

Choose a reason for hiding this comment

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

minimum required version for TS 5.5

@chrisvdp
Copy link
Author

@SergeAstapov should I update the matrix of TS versions this runs against?

https://github.com/tracked-tools/tracked-built-ins/blob/master/.github/workflows/CI.yml#L144

Was thinking along the lines of

ts-version:
          - '5.2'
          - '5.3'
          - '5.4'
          - '5.5'
          - '5.6'
          - 'next'

@SergeAstapov
Copy link
Collaborator

@chrisvdp if you don't mind, I would separate infra updates and TS support matrix from this specific fix.

Let me work on bringing CI in this repo to green state and we can get back to this specific fix to ensure we iterate on green CI and do one thing at a time.

@chrisvdp
Copy link
Author

@chrisvdp if you don't mind, I would separate infra updates and TS support matrix from this specific fix.

Let me work on bringing CI in this repo to green state and we can get back to this specific fix to ensure we iterate on green CI and do one thing at a time.

Sounds good

@NullVoxPopuli
Copy link
Collaborator

#424 should unblock this

@NullVoxPopuli
Copy link
Collaborator

this is unblocked!

@chrisvdp
Copy link
Author

this is unblocked!

Rebased

@SergeAstapov SergeAstapov changed the title feat: fix set issues for TS 5.5+ feat: fix set issues for TypeScript 5.5+ Nov 28, 2024
@SergeAstapov
Copy link
Collaborator

@chrisvdp CI is failing with 5.4 and 5.5, mind to take a look if this can be fixed?

@NullVoxPopuli
Copy link
Collaborator

Should we drop support for TS < 5.6?

@chrisvdp
Copy link
Author

I think that at least >=5.5 is required as we are adding features added there. I think I can fix the issues with 5.5 by removing some of the explicit return types as they seem to have changed between 5.5 -> 5.6

@chrisvdp
Copy link
Author

I pushed a commit that works on 5.5 and 5.6 locally for me

@SergeAstapov
Copy link
Collaborator

Should we drop support for TS < 5.6?

fine for me. then, we could drop support for some older Ember.js versions as well to bundle into next major

@SergeAstapov
Copy link
Collaborator

I'd like to drop 5.4 support in separate PR so this one would be focused on fixing Set issues

@NullVoxPopuli
Copy link
Collaborator

@SergeAstapov here you go: #434

@chrisvdp so so so sorry this has taken so long -- once the above is merged, one last rebase should be all we need <3

@NullVoxPopuli
Copy link
Collaborator

we're good to go! @chrisvdp do you happen to have time to rebase?

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

Successfully merging this pull request may close these issues.

3 participants