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 types for Jest modules #33705

Closed
wants to merge 11 commits into from
Closed

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Mar 7, 2019

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If removing a declaration:

  • If a package was never on DefinitelyTyped, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
  • Delete the package's directory.
  • Add it to notNeededPackages.json.

Jest 24.3.0 has been released, and is completely rewritten to TypeScript, and distributes types. There might be subtle differences between the ones in this repo and the official ones, but I'm hopeful people will PR us the changes they need 🙂

Not sure what this means for

/// <reference types="jest-diff" />

Does it need a package.json now?

EDIT: CI failed without it, so added one

One thing to note is that expect also has typings, but the expect typings in this repo is for https://github.com/mjackson/expect, not the one from jest. However, the expect typings do not include any actual matchers, so we should fix that first

@SimenB
Copy link
Contributor Author

SimenB commented Mar 7, 2019

Oh btw, @types/jest itself is left alone. It could maybe import submodules (there's a lot of duplication there), but we're not ready to distribute a typings file with the globals etc. So it should not be considered deprecated quite yet 🙂

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Mar 7, 2019
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Mar 7, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 7, 2019

@SimenB Thank you for submitting this PR!

🔔 @jmreidy @merrywhether @NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @ahnpnl @JoshuaKGoldberg @UselessPickles @r3nya @Hotell @sebald @andys8 @antoinebrault @lifeiscontent @myabc @theutz @nickmccurdy - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@SimenB
Copy link
Contributor Author

SimenB commented Mar 7, 2019

@sandersn I'm a bit out of my depth on the CI failure 😬

@sandersn
Copy link
Contributor

sandersn commented Mar 7, 2019

@SimenB You need to clone Microsoft/types-publisher and add expect to dependenciesWhitelist.txt. I'll merge it and publish a new version of types-publisher. Then CI will pass on a re-run.

npm install @types/jest now includes expect as a dependency, which could run code on install. We don't want to add dependencies without thinking about them first.

@SimenB
Copy link
Contributor Author

SimenB commented Mar 8, 2019

Cool, thanks! microsoft/types-publisher#589

@sandersn
Copy link
Contributor

sandersn commented Mar 8, 2019

@SimenB OK, I re-ran CI and it looks like there are a couple of errors but they seem to be more to do with linting and correct versions of expect now.

@SimenB
Copy link
Contributor Author

SimenB commented Mar 8, 2019

The errors confuse me.

jest/index.d.ts(358,50): error TS2694: Namespace 'expect' has no exported member 'MatcherState'.

https://unpkg.com/expect@24.3.0/build/index.d.ts

I also copied this file into a project of mine and it worked

@SimenB
Copy link
Contributor Author

SimenB commented Mar 8, 2019

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Mar 8, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 8, 2019

@SimenB The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@sandersn
Copy link
Contributor

sandersn commented Mar 8, 2019

Yes. It looks in ../expect before looking for node_modules/expect, because of typeRoots (or baseUrl?) in tsconfig.json, which is by design.

@types/expect looks very out of date and expect ships its own types now. I guess that means you should remove @types/expect too.

I've been meaning to do a cleanup run to remove all @types packages that have source packages with types, but I haven't had a chance yet.

@SimenB
Copy link
Contributor Author

SimenB commented Mar 9, 2019

Should make the expect types better first (they don't have the matchers). I'll do that, then come back to you 🙂

@typescript-bot
Copy link
Contributor

@SimenB I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

@SimenB
Copy link
Contributor Author

SimenB commented Mar 14, 2019

New release of expect has the matchers now, so I've updated this

@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 14, 2019

@SimenB The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@SimenB
Copy link
Contributor Author

SimenB commented Mar 14, 2019

A bunch of TS1192: Module '"/home/travis/build/DefinitelyTyped/DefinitelyTyped/types/jest/node_modules/jest-diff/build/index"' has no default export. which is correct - it's a export =. How can I fix that?

@sandersn
Copy link
Contributor

@SimenB All the ones I saw looked like this:

../jest/node_modules/jest-matcher-utils/build/index.d.ts(7,8): 
error TS1192: 
Module '".../DefinitelyTyped/types/jest/node_modules/jest-diff/build/index"' has no default export

which indicates that jest-matcher-utils expects jest-diff to have export default instead of export =, which is the case (see the code below). Probably jest-matcher-utils needs to be updated to use (1) import jestDiff = require('jest-diff') or (2) set "esModuleInterop": true.

I guess that everybody who uses jest-matcher-utils in Typescript without esModuleInterop is seeing this error right now, because neither types are DT types.

// @Filename: jest-diff/built/index.d.ts
import { DiffOptions as JestDiffOptions } from './types';
declare function diff(a: any, b: any, options?: JestDiffOptions): string | null;
declare namespace diff {
    type DiffOptions = JestDiffOptions;
}
export = diff;
// @Filename: jest-matcher-utils/build/index.d.ts
import jestDiff, { DiffOptions } from 'jest-diff';

@SimenB
Copy link
Contributor Author

SimenB commented Mar 14, 2019

Aha, we use "esModuleInterop": true indeed.

https://github.com/facebook/jest/blob/bdeb5af8ce1cb8e372d4eb51612f711a2979e572/tsconfig.json#L23

What's preferred, Jest changing to import blah = require() or changing config here?

Potentially, we could wait until Jest 25 where we can make breaking changes to the export and use actual ES module exports instead of the hacky half-solution we have now to keep CJS compat

@sandersn
Copy link
Contributor

Well, d.ts files that ship to customers should not assume that they will have "esModuleInterop": true, so the safe option is to use import = require.

@typescript-bot typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Mar 21, 2019
@typescript-bot
Copy link
Contributor

@SimenB To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

@despairblue
Copy link
Contributor

@SimenB Hey, what's the state on this? We're hitting exactly what was described here: #33705 (comment)

Using esModuleInterop actually leads to hundreds of other errors with other dependencies:

Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead.

@SimenB
Copy link
Contributor Author

SimenB commented May 3, 2019

We'll be changing imports and exports to be esm for jest 25

@despairblue
Copy link
Contributor

@SimenB Thanks, for the time being we migrated all our project to use esModuleInterop and allowSyntheticDefaultImports.

Is there an issue for this that I can subscribe to?

@SimenB SimenB deleted the jest-typings branch January 24, 2020 15:24
@SimenB SimenB mentioned this pull request Jan 24, 2020
9 tasks
@SimenB
Copy link
Contributor Author

SimenB commented Jan 24, 2020

Resurrected in #41837

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned This PR had no activity for a long time, and is considered abandoned Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants