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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(jest-types): make ConfigGlobal an interface #9570

Merged
merged 5 commits into from Feb 28, 2020
Merged

fix(jest-types): make ConfigGlobal an interface #9570

merged 5 commits into from Feb 28, 2020

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Feb 13, 2020

As discussed here.

Summary

Makes ConfigGlobal an interface to allow for declaration merging:

// ts-jest/index.d.ts
declare module '@jest/types' {
  namespace Config {
    interface ConfigGlobals {
      'ts-jest': TsJestGlobalOptions;
    }
  }
}

// jest.config.js
/** @type {import('@jest/types').Config.InitialOptions} */
const config= {
  globals: {
    'ts-jest': {
      isolatedModules: true,
      myProperty: 1 // error, "myProperty" is not defined!
    }
  }
}

module.exports = config;

Test plan

None 馃槵

@@ -27,7 +27,9 @@ export type HasteConfig = {
export type ReporterConfig = [string, Record<string, unknown>];
export type TransformerConfig = [string, Record<string, unknown>];

export type ConfigGlobals = Record<string, any>;
export interface ConfigGlobals {
[K: string]: any;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[K: string]: any;
[K: string]: unknown;

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'll be the ideal; let's commit and see if anything breaks :D

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder if we should call it global instead of K?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

K is typically the convention for this pattern, as it's a generic that represents the property name.

i.e

/**
 * Construct a type with a set of properties K of type T
 */
type Record<K extends keyof any, T> = {
    [P in K]: T;
};

I'm not fussed, but the actual name won't show up much anyway.

Copy link
Member

Choose a reason for hiding this comment

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

It's not really generic though, it's a string. but it doesn't matter like you say 馃檪


seems this introduced a type error in normalize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah so the proper type should be object, due to:

const mergeGlobalsWithPreset = (
  options: Config.InitialOptions,
  preset: Config.InitialOptions,
) => {
  if (options['globals'] && preset['globals']) {
    for (const p in preset['globals']) {
      options['globals'][p] = {
        ...preset['globals'][p],
        ...options['globals'][p],
      };
    }
  }
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB so we've got a couple of options, depending on how you're feeling:

Looking at the docs, I think any maybe the "correct" type?

A set of global variables that need to be available in all test environments.

For example, the following would create a global __DEV__ variable set to true in all test environments:

Poking around a bit in normalize, I can't actually find what stops jest from crashing with an error about ... ?

Today I learned that you can actually spread a boolean without erroring O.o

Copy link
Member

Choose a reason for hiding this comment

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

To me it looks like the code is broken, and mergeGlobalsWithPreset will discard e.g. the __DEV__: true example from the docs? It should use a proper recursive merge, not blindly spread into one another

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll adjust the implementation.

Copy link
Contributor Author

@G-Rath G-Rath Feb 15, 2020

Choose a reason for hiding this comment

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

Ah, finally made it give bad output:
image

@@ -27,7 +27,9 @@ export type HasteConfig = {
export type ReporterConfig = [string, Record<string, unknown>];
export type TransformerConfig = [string, Record<string, unknown>];

export type ConfigGlobals = Record<string, any>;
export interface ConfigGlobals {
[K: string]: object;
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 unknown is correct, we just need to properly merge the config provided rather than blindly spreading

Copy link
Contributor Author

@G-Rath G-Rath Feb 16, 2020

Choose a reason for hiding this comment

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

Yeah for sure - I've got a test and everything; just need to come up with a merging solution.

One of the jest packages already has one coded but not exported - I could move that into jest-utils (iirc that's a package that exists?) a shared package.

Otherwise deepMerge is used by a few packages, so I'll probably just stick that there 馃し鈥嶁檪

@codecov-io
Copy link

Codecov Report

Merging #9570 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9570      +/-   ##
==========================================
- Coverage   65.09%   65.09%   -0.01%     
==========================================
  Files         287      287              
  Lines       12145    12144       -1     
  Branches     3007     3009       +2     
==========================================
- Hits         7906     7905       -1     
  Misses       3604     3604              
  Partials      635      635
Impacted Files Coverage 螖
packages/jest-config/src/normalize.ts 76.66% <100%> (-0.07%) 猬囷笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 4f4513b...75407fe. Read the comment docs.

@SimenB SimenB merged commit cc5d630 into jestjs:master Feb 28, 2020
@SimenB
Copy link
Member

SimenB commented Feb 28, 2020

ended up being a bug fix, nice 馃榾 any sucks

@G-Rath G-Rath deleted the make-ConfigGlobals-an-interface branch February 29, 2020 04:55
@G-Rath
Copy link
Contributor Author

G-Rath commented Feb 29, 2020

any sucks

It does indeed :/

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants