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

ci: Remove dtslint #2435

Closed
wants to merge 4 commits into from
Closed

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Feb 9, 2025

Pull Request

Issue

Remove types check; reason see parse-community/parse-server#9581.

Approach

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title ci: remove dtslint ci: Remove dtslint Feb 9, 2025
Copy link

parse-github-assistant bot commented Feb 9, 2025

Thanks for opening this pull request!

  • ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

Copy link

codecov bot commented Feb 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (fa34827) to head (8f51a54).
Report is 18 commits behind head on alpha.

Additional details and impacted files
@@            Coverage Diff            @@
##             alpha     #2435   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        64           
  Lines         6256      6256           
  Branches      1452      1478   +26     
=========================================
  Hits          6256      6256           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -102,7 +101,6 @@
"test": "cross-env PARSE_BUILD=node jest",
"test:mongodb": "npm run test:mongodb:runnerstart && npm run integration",
"test:mongodb:runnerstart": "mongodb-runner start -- --port 27017",
"test:types": "dtslint --expectOnly types",
Copy link
Member

Choose a reason for hiding this comment

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

I think I figured out how to enable full typescript support with shipping types to npm. I was waiting for #2425 to be merged so we don’t have conflicts

@dplewis
Copy link
Member

dplewis commented Feb 9, 2025

@dblythy the reason why dtslint is here is because we haven’t moved away from definitelyTyped yet. Once we move away then this can be removed

@mtrezza
Copy link
Member

mtrezza commented Feb 9, 2025

@dplewis I've added the link in the issue description for why we're removing this. The package doesn't seem to be intended to be used that way and we ran into issues in Parse Server. If you follow the thread you'll find people hacking stuff together to make this work somehow. Even though it seems to work in this repo for now, we're removing it preemptively to rely solely on the types build step not to fail for the CI job to pass.

@dplewis
Copy link
Member

dplewis commented Feb 9, 2025

@mtrezza go for it

@mtrezza
Copy link
Member

mtrezza commented Feb 9, 2025

Do you have any concerns?

@dplewis
Copy link
Member

dplewis commented Feb 12, 2025

@dblythy @mtrezza I'd recommend keeping this until someone can get the type test suite to pass. There is a Cannot find namespace 'Parse' error when using at the top level that is stopping a typescript release.

Here is the test suite for reference copied from DT
https://github.com/parse-community/Parse-SDK-JS/blob/alpha/types/tests.ts

Edit: It should be easy to replace dtslintwith eslint-plugin-expect-type

@dplewis
Copy link
Member

dplewis commented Feb 15, 2025

@dblythy I created a PR #2452 to show case my last comment. I agree dtslint should be removed in the future.

@mtrezza
Copy link
Member

mtrezza commented Feb 16, 2025

PR on hold; see #2452 (comment)

@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants