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

[Umbrella] Updating & Fixing Types #8436

Closed
5 of 8 tasks
JoshRosenstein opened this issue May 7, 2019 · 12 comments
Closed
5 of 8 tasks

[Umbrella] Updating & Fixing Types #8436

JoshRosenstein opened this issue May 7, 2019 · 12 comments

Comments

@JoshRosenstein
Copy link
Contributor

JoshRosenstein commented May 7, 2019

☔️ Overview

Doing a clean up round on types, using this issue to track the PR's associated to it.Will be editing this post as I progress. Comments/Recommendations are appreciated.

First Round

First Round will be small isolated PR's, that way no conflicting discussions or delays in certain scope effects the initiation of Second Round Changes.

Second Round

Second Round will begin after key PRs(dependable) from first round are committed.

  • jest-config(normalize) - Improve typing's and structure
@SimenB
Copy link
Member

SimenB commented May 7, 2019

Thank you so much! ❤️

@SimenB
Copy link
Member

SimenB commented May 7, 2019

Regarding normalize, it'd be great to pass it through io-ts, runtypes or something similar instead of rolling our own thing.

@jeysal probably has opinions on this 🙂

@JoshRosenstein
Copy link
Contributor Author

Thank you so much! ❤️

np, cant think of a better way to practice/learn typescript 😃
@SimenB You mind changing the labels here?

@JoshRosenstein
Copy link
Contributor Author

JoshRosenstein commented May 7, 2019

@SimenB In terms of the validation part? Or looking for a 3rd parties to handle then entire validation and option merging/overriding?

The biggest issue I see in normalize currently is how its creating the final newOption Result. not type checking (Config.InitialOptions[option] === AllOptions[key]) or even( keyof InitialOptions === keyof AllOptions) (because they do differ ) ie:'resolver','mapCoverage','scriptPreprocessor','json','preprocessorIgnorePatterns','preset','setupTestFrameworkScriptFile','testPathDirs','testSequencer','updateSnapshot' exists in Initial Options, and either doesn't exists in alloptions, or they are transformed prior to the reducer, but at the end, these still get set within the resulting Option.

Is the simplfied api aiming to be rolled out on jest 25?

Regarding normalize, it'd be great to pass it through io-ts, runtypes or something similar instead of rolling our own thing.

@jeysal probably has opinions on this 🙂

@jeysal
Copy link
Contributor

jeysal commented May 7, 2019

@JoshRosenstein
My preference would be to validate user-provided config using io-ts. Then, once we've ensured that the objects we're operating on actually match our static types exactly, we should generate the internal config format in a way that typechecks well (e.g. avoiding mutation in favour of a more functional style usually helps with this).
To be clear, I still see the need to treat user config and internal config separately. One reason for this is that internal config usually has goals like not being redundant (e.g. you shouldn't need to both check for undefined and '' on the config unless there's a semantic difference), while user config might not care about this and offer alternatives (e.g. shortening an array containing a single string to just a string) - these are just examples.
Simplifying configuration is scheduled for v25, yes. I expect normalize to be mostly rewritten in the process (kind of scared how this will play with not reworking CLI options at the same time though), so I think the easiest way of going about it would be to just ensure that the new code is TSC-friendly.

@JoshRosenstein
Copy link
Contributor Author

JoshRosenstein commented May 8, 2019

@jeysal , that makes sense. and in that case i will hold off on any dramatic changes on normalize. On a related note,I was trying to think of a way to re-format jest-types Config, and this may also be helpful when restructuring the way the configs are handled.
With the current state of Config types, i was trying to think of a way to break down the types and reuse them throughout the main exports InitialOptions,DefaultOptions ,GlobalConfig ProjectConfig Argv. Rather than the current setup of having all properties repeatedly typed. So I began with partitioning the overlaps of GlobalConfig ProjectConfig Argv, and treated the properties as a pipe line from argv. and came up with something like this:
*"Shared" refers to ProjectConfig & GlobalConfig
Looking at ProjectConfig
First, what properties types on Argv, are identical to the required ProjectConfig, (will call this Direct)

/** Argv Options That share same type as Project Options*/
export interface ArgvToProjectDirect {
  automock: boolean;
  browser: boolean;
  cache: boolean;
  cacheDirectory: Path;
  clearMocks: boolean;
  moduleDirectories: Array<string>;
  moduleFileExtensions: Array<string>;
  modulePathIgnorePatterns: Array<string>;
  modulePaths: Array<string>;
  preset: string | Nil;
  prettierPath: string;
  resetMocks: boolean;
  resetModules: boolean;
  resolver: Path | Nil;
  restoreMocks: boolean;
  roots: Array<Path>;
  setupFiles: Array<Path>;
  setupFilesAfterEnv: Array<Path>;
  snapshotSerializers: Array<Path>;
  testEnvironment: string;
  testPathIgnorePatterns: Array<string>;
  testRunner: string;
  testURL: string;
  transformIgnorePatterns: Array<Glob>;
  unmockedModulePathPatterns: Array<string> | Nil;
  watchPathIgnorePatterns: Array<string>;
}
}

⬇️ Next what are the properties that share the same key but undlerying types are different. ( will break this down into 2 Raw & derivedfromArgv.

/** Argv Options needed for Project Options that require manipulation*/
export interface ArgvToProjectRaw {
  globals: string;
  haste: string;
  moduleNameMapper: string;
  testRegex: string | Array<string>;
  timers: string;
  transform: string;
}

/** The Project Options Derived from Argv Options */
export interface ProjectOptionsDerivedFromArgv {
  globals: ConfigGlobals;
  haste: HasteConfig;
  moduleNameMapper: Array<[string, string]>;
  testRegex: Array<string>;
  timers: 'real' | 'fake';
  transform: Array<[string, Path]>;
}

⬇️ Next the remaining ProjectOptions that are derived elsewhere or in combination of other options, (will call this ProjectOptionsDerived)

export interface ProjectOptionsDerived {
  cwd: Path;
  dependencyExtractor?: string;
  displayName?: DisplayName;
  forceCoverageMatch: Array<Glob>;
  moduleLoader: Path;
  name: string;
  runner: string;
  skipFilter: boolean;
  skipNodeResolution: boolean;
  snapshotResolver: Path | Nil;
  testEnvironmentOptions: Record<string, any>;
  testLocationInResults: boolean;
}

Now do the same for GlobalConfig

/** The Argv Options shared to directly to global */
export interface ArgvToGlobalDirect {
  changedFilesWithAncestor: boolean;
  changedSince: string;
  collectCoverage: boolean;
  coverageDirectory: string;
  findRelatedTests: boolean;
  forceExit: boolean;
  json: boolean;
  lastCommit: boolean;
  logHeapUsage: boolean;
  maxWorkers: number;
  noStackTrace: boolean;
  notify: boolean;
  onlyChanged: boolean;
  projects: Array<Glob>;
  silent: boolean;
  testMatch: Array<Glob>;
  testNamePattern: string;
  testResultsProcessor: string | Nil;
  testSequencer: string;
  useStderr: boolean;
  verbose: boolean | Nil;
  watch: boolean;
  watchAll: boolean;
  watchman: boolean;
}

/** Argv Options needed for Global Options that require manipulation*/
export interface ArgvToGlobalRaw {
  bail: boolean | number;
  collectCoverageFrom: string;
  collectCoverageOnlyFrom: Array<string>;
  coverageReporters: Array<string>;
  coverageThreshold: string;
  outputFile: Path;
  testPathPattern: Array<string>;
  testFailureExitCode: string | Nil;
  notifyMode: string;
  updateSnapshot: boolean;
}

/** The Global Options Derived from Argv Options */
export interface GlobalOptionsDerivedFromArgv {
  bail: number;
  collectCoverageFrom: Array<Glob>;
  collectCoverageOnlyFrom: {[key: string]: boolean} | Nil;
  coverageReporters: Array<ReportOptionKeys>;
  coverageThreshold: CoverageThreshold;
  expand: boolean;
  notifyMode: NotifyMode;
  outputFile: Path | Nil;
  testFailureExitCode: number;
  testPathPattern: string;
  updateSnapshot: SnapshotUpdateState;
}

/** Derived Global Options*/
export interface GlobalOptionsDerived {
  enabledTestsMap: {[key: string]: {[key: string]: boolean}} | Nil;
  forceExit: boolean;
  listTests: boolean;
  maxConcurrency: number;
  noSCM: boolean | Nil;
  nonFlagArgs: Array<string>;
  onlyFailures: boolean;
  passWithNoTests: boolean;
  replname: string | Nil;
  reporters: Array<string | ReporterConfig>;
  runTestsByPath: boolean;
  skipFilter: boolean;
  watchPlugins: Array<{path: string; config: Record<string, any>}> | Nil;
}

Then again for options that are shared b/n global and project

/** Argv Options That share same type as Shared Project & Global Options */
export interface ArgvToSharedDirect {
  coveragePathIgnorePatterns: Array<string>; // Global has optional
  globalSetup: string | Nil;
  globalTeardown: string | Nil;
  rootDir: Path;
}

/** Argv Options needed for Project & Global Options that require manipulation*/
export interface ArgvToSharedRaw {}

/** Shared Project & Global Options Derived from Argv Options */
export interface SharedOptionsDerivedFromArgv {}

/** Derived Options that are shared b/n Global and Project*/
export interface SharedOptionsDerived {
  detectLeaks: boolean;
  detectOpenHandles: boolean;
  errorOnDeprecated: boolean;
  extraGlobals: Array<keyof NodeJS.Global>; /// GlobalConfig has string[]
  filter: Path | Nil;
}

Now defining the remaining argv config that doesnt get passed on

/** Argv Options That dont share same config names*/
export interface ArgvCustom {
  /** This Option Does...*/
  all: boolean;
  ci: boolean;
  clearCache: boolean;
  color: boolean;
  colors: boolean;
  config: string;
  coverage: boolean;
  debug: boolean;
  env: string;
  init: boolean;
  runInBand: boolean;
  showConfig: boolean;
  version: boolean;
}

Then the final resulting types:

export interface ArgVRaw
  extends ArgvCustom,
    ArgvToGlobalRaw,
    ArgvToGlobalDirect,
    ArgvToSharedDirect,
    ArgvToSharedRaw,
    ArgvToProjectDirect,
    ArgvToProjectRaw {}

export type Argv = Arguments<Partial<ArgVRaw>>;
export interface SharedOptions
  extends SharedOptionsDerived,
    SharedOptionsDerivedFromArgv,
    ArgvToSharedDirect {}

export interface SharedOptions
  extends SharedOptionsDerived,
    SharedOptionsDerivedFromArgv,
    ArgvToSharedDirect {}

export interface GlobalOptions
  extends SharedOptions,
    GlobalOptionsDerived,
    GlobalOptionsDerivedFromArgv,
    ArgvToGlobalDirect {}

export interface ProjectOptions
  extends SharedOptions,
    ProjectOptionsDerived,
    ProjectOptionsDerivedFromArgv,
    ArgvToProjectDirect {}

export interface AllOptions extends GlobalOptions, ProjectOptions {}

export type AllOptionsPartial = Partial<AllOptions>;

export type AllOptionsNullable = {[P in keyof AllOptions]: AllOptions[P] | Nil};

// Allow Setting Default to Null ?
export type DefaultOptions = Partial<AllOptionsNullable>;

export type InitialOptions = AllOptionsPartial;

Breaking them down may also be helpful with functional transformers:

const someTransformer= (input:ArgvToProjectRaw):ProjectOptionsDerivedFromArgv=>...

And you will also be able to write option api description once and keep intelli throughout.
config_intelli

lmk your thoughts... i'll tinker around a bit. (btw sorry for the long tangent)

@jeysal
Copy link
Contributor

jeysal commented May 9, 2019

@JoshRosenstein Nice assessment, I think this writeup is very useful already for knowing what kind of options and transformations we actually have.
Deduplicating config definitions is definitely also a goal when cleaning up the typings here. I originally thought we'd likely end up using Pick a lot to avoid duplicating property types, but something like this might actually be cleaner because it also deduplicates the config keys 🤔

@JoshRosenstein
Copy link
Contributor Author

JoshRosenstein commented May 9, 2019

@jeysal
Thanks, after looking over again, its not 100% correct, but you can get the gist of it. The naming convention got me a confused at first (intialOptions defaultOptions, projectConfig, globalConfig), because I now realize intialOption represents the allowed types from userConfig files and defaultOptions should represent projectConfig & globalConfig which reflect the finalized types modules will consume.

So the pipeline would be more look like Argv=>UserConfigs=>FinalOptions, and each broken down b/n Global & Project with transformation branches.

Also we can then remove DefaultOptions type, and use pick to validate defaultOptions exported from [jest-config]-Defaults.

const defaultOptions: Pick<Config.FinalOptions, DefaultOptionKeys> = {
  automock: false,
  bail: 0,
  browser: false,

Just using FinalOptions to be clear in this post, but do you think we should change the current naming convention ?

@jeysal
Copy link
Contributor

jeysal commented May 12, 2019

I can see how the current naming is confusing, although I think Config is fine - that's where most of it will come from, options sounds more like CLI to me - and DefaultOptions is the weird one. Rewrite should definitely include some bikeshedding on the naming 😅

@JoshRosenstein
Copy link
Contributor Author

Yeah especially for those that write app presets in typescript, being able to know which type they need to use lol. Since the file name is jest.config, a simple JestConfig to replace initialOptions would be prob be best , And I guess DefaultConfig being the defaults for ProjectConfig & GlobalConfig. 🤢

@JoshRosenstein
Copy link
Contributor Author

Closing this. Currently unable to continue contributing.

@github-actions
Copy link

This issue 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

No branches or pull requests

3 participants