Skip to content

Conversation

@mshustov
Copy link
Contributor

@mshustov mshustov commented Nov 19, 2020

Problem is reported in
#82300 (comment)

Output in the current configuration:
packages/kbn-i18n/target/types/react/index.d.ts

import { InjectedIntl as _InjectedIntl, InjectedIntlProps as _InjectedIntlProps } from 'react-intl';
export declare type InjectedIntl = _InjectedIntl;
export declare type InjectedIntlProps = _InjectedIntlProps;
export { intlShape, FormattedDate, FormattedTime, FormattedRelative, FormattedNumber, FormattedPlural, FormattedMessage, FormattedHTMLMessage, IntlProvider as __IntlProvider, } from 'react-intl';
export { I18nProvider } from './provider';
export { injectI18n } from './inject';
//# sourceMappingURL=index.d.ts.map

packages/kbn-test/target/types/jest/utils/enzyme_helpers.d.ts

/// <reference types="cheerio" />
/// <reference types="jest" />
import { ReactWrapper } from 'enzyme';
import React, { ReactElement, ValidationMap } from 'react';
/**
 *  Creates the wrapper instance using shallow with provided intl object into context
 *
 *  @param node The React element or cheerio wrapper
 *  @param options properties to pass into shallow wrapper
 *  @return The wrapper instance around the rendered output with intl object in context
 */
export declare function shallowWithIntl<T>(node: ReactElement<T>, { context, childContextTypes, ...props }?: {
    context?: any;
    childContextTypes?: ValidationMap<any>;
}): import("enzyme").ShallowWrapper<T & {
    intl: ReactIntl.InjectedIntl;
}, Readonly<{}>, React.Component<{}, {}, any>>;
// ...

Note that InjectedIntl type is not imported from @kbn/i18n/react, but rather referenced by the value. I saw a similar problem reported in https://www.techatbloomberg.com/blog/10-insights-adopting-typescript-at-scale/
It doesn't explain why we don't see any problems on master, but on #82300 PR ReactIntl cannot be inferred. Anyway, the global namespace is not something we want to rely on, so there are 2 options:

  • switch from type to interface
  • export the namespace explicitly

for both options the InjectedIntl imported explicitly in the emitted d.ts file:
packages/kbn-test/target/types/jest/utils/enzyme_helpers.d.ts

/// <reference types="cheerio" />
/// <reference types="jest" />
/**
 * Components using the @kbn/i18n module require access to the intl context.
 * This is not available when mounting single components in Enzyme.
 * These helper functions aim to address that and wrap a valid,
 * intl context around them.
 */
import { InjectedIntl } from '@kbn/i18n/react';
import { ReactWrapper } from 'enzyme';
import React, { ReactElement, ValidationMap } from 'react';
/**
 *  Creates the wrapper instance using shallow with provided intl object into context
 *
 *  @param node The React element or cheerio wrapper
 *  @param options properties to pass into shallow wrapper
 *  @return The wrapper instance around the rendered output with intl object in context
 */
export declare function shallowWithIntl<T>(node: ReactElement<T>, { context, childContextTypes, ...props }?: {
    context?: any;
    childContextTypes?: ValidationMap<any>;
}): import("enzyme").ShallowWrapper<T & {
    intl: InjectedIntl;
}, Readonly<{}>, React.Component<{}, {}, any>>;

As a follow up, I'm going to reproduce the problem on a smaller codebase and reach out the TS team. We might want to adopt Prefer interfaces eslint rule. It seems to be an approach recommended by TS team https://twitter.com/drosenwasser/status/1319205169918144513

@mshustov mshustov added chore Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Team:Operations Kibana-Operations Team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 19, 2020
@mshustov mshustov requested a review from a team as a code owner November 19, 2020 14:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)


export type InjectedIntl = _InjectedIntl;
export type InjectedIntlProps = _InjectedIntlProps;
export type { InjectedIntl, InjectedIntlProps } from 'react-intl';
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, I can't rationalize how this is actually different from the previous code... This seems like a bug in TS and I'm glad you hear you plan to report it to the TS team... I'm shocked that you figured this out.

Copy link
Member

Choose a reason for hiding this comment

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

Very odd indeed.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mshustov mshustov merged commit ec983ef into elastic:master Nov 20, 2020
@mshustov mshustov deleted the no-inline-types-i18n branch November 20, 2020 14:27
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 20, 2020
* master: (38 commits)
  [ML] Data frame analytics: Adds functionality to map view (elastic#83710)
  Add usage collection for savedObject tagging (elastic#83160)
  [SECURITY_SOLUTION] 145: Advanced Policy Tests (elastic#82898)
  [APM] Service overview transactions table (elastic#83429)
  [ML] Fix Single Metric Viewer not loading if job is metric with no partition (elastic#83880)
  do not export types from 3rd party modules as 'type' (elastic#83803)
  [Fleet] Allow to send SETTINGS action (elastic#83707)
  Fixes Failing test: Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/details·ts - Actions and Triggers app Alert Details Alert Instances renders the active alert instances (elastic#83478)
  [Uptime]Reduce chart height on monitor detail page (elastic#83777)
  [APM] Prefer `APIReturnType` over `PromiseReturnType` (elastic#83843)
  [Observability] Fix telemetry for Observability Overview (elastic#83847)
  [Alerting] Adds generic UI for the definition of conditions for Action Groups (elastic#83278)
  ensure workload agg doesnt run until next interval when it fails (elastic#83632)
  [ILM] Policy form should not throw away data (elastic#83077)
  [Monitoring] Stop collecting Kibana Usage in bulkUploader (elastic#83546)
  [TSVB] fix wrong imports (elastic#83798)
  [APM] Correlations UI POC (elastic#82256)
  list all the refs in  tsconfig.json (elastic#83678)
  Bump jest (and related packages) to v26.6.3 (elastic#83724)
  Functional tests - stabilize reporting tests for cloud execution (elastic#83787)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported chore release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Team:Operations Kibana-Operations Team v7.11.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants