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

chore: add a .gitignore in types folder to not allow pushing simple .ts-files, add check-types-filename.js #11516

Merged
merged 7 commits into from
Mar 30, 2022

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Mar 10, 2022

This is a PR following #11513
With this platform-independent solution, it is not possible to check-in simple .ts files in the types folder. It still can be forced with git add -f ./types/example.ts, but it is a "cheap" solution.
To prevent a force add, I added a check-types-extensions.js, which checks if we did not accidently added a simple .ts file.

This should effectively prevent again a regression like it happened in 6.2.5.

Till #11513 is merged, this should correctly fail. After #11513 is merged, it should pass.

@vkarpov15
Copy link
Collaborator

I'm just surprised, why did tsd not catch this?

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Mar 11, 2022

I think when tsd uses typescript under the hood it can resolve .d.ts and .ts files. But if we import typings in a typescript project, typescript goes another route and then it can only find .d.ts files.

added also a check that the filename should not contain uppercase characters to avoid also a regression like #11468

@Uzlopak Uzlopak changed the title chore: add a .gitignore in types folder to not allow pushing simple .ts-files, add check-types-extensions.js chore: add a .gitignore in types folder to not allow pushing simple .ts-files, add check-types-filename.js Mar 11, 2022
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Mar 11, 2022

I opened an issue in tsd regarding the latest regressions

tsdjs/tsd#145

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

I'm not thrilled by this solution because I think it is disconnected from what we're actually trying to test. What we're trying to test here is that Mongoose compiles when used with TypeScript. This additional check catches one case that could cause an error, but doesn't catch others.

I'm leaning toward adding a minimal version of TypeScript compilation tests similar to what we removed when we switched to tsd. That approach can have some advantages:

  1. Make sure we don't accidentally ship something that doesn't compile
  2. Let us test that Mongoose at least compiles against different versions of TypeScript

What do you think @Uzlopak ?

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

I thought a bit more about this and I'll just err on the side of merging this for now. This PR does the job of preventing shipping code with broken TS references and isn't too cumbersome.

@vkarpov15 vkarpov15 added this to the 6.2.10 milestone Mar 30, 2022
@vkarpov15 vkarpov15 merged commit 61a8089 into Automattic:master Mar 30, 2022
@Uzlopak Uzlopak deleted the gitignore-ts-files-in-types branch April 5, 2022 23:58
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