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

Attempt to fix incremental build error #84152

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

mshustov
Copy link
Contributor

Summary

From time to time yarn kbn bootstrap fails with a cryptic error:

src/plugins/usage_collection/server/collector/collector_set.ts:109:41 - error TS2345: Argument of type 'Collector<TFetchReturn, TFormatForBulkUpload, ExtraOptions>' is not assignable to parameter of type 'AnyCollector'.
              Types of property 'fetch' are incompatible.
                Type 'CollectorFetchMethod<any, TFetchReturn, ExtraOptions>' is not assignable to type 'CollectorFetchMethod<any, any, {}>'.
                  Type '{}' is not assignable to type 'ExtraOptions'.
                    '{}' is assignable to the constraint of type 'ExtraOptions', but 'ExtraOptions' could be instantiated with a different subtype of constraint 'object'.
            109     this.collectors.set(collector.type, collector);
                                                        ~~~~~~~~~

At the same time, neither VSCode nor Webstorm shows the errors. It makes me think there is a problem with type inference for the Collector class. I tried to isolate an error case, but the complexity of type generics in https://github.com/elastic/kibana/blob/030343d9320bf22721b18ffdb45a063b5f391298/src/plugins/usage_collection/server/collector/collector.ts is not trivial. So I assumed that problem might be with the fetch type itself not satisfying the constraints.

@mshustov mshustov added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 23, 2020
@mshustov mshustov requested a review from a team as a code owner November 23, 2020 20:13
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Reading the error, it looks to me like TS complains about this line:
https://github.com/restrry/kibana/blob/attepmpt-to-fix-floating-type-error/src/plugins/usage_collection/server/collector/collector_set.ts#L109

In that one, we are setting the Collector<TFetchReturn, TFormatForBulkUpload, ExtraOptions> to a Map<string, AnyCollector>.

AnyCollector is Collector<any, any> (with TS turns into Collector<any, any, {}>).

So, essentially, we are trying to assign Collector<TFetchReturn, TFormatForBulkUpload, ExtraOptions> to Collector<any, any, {}>.

Since the error claims Type '{}' is not assignable to type 'ExtraOptions', I think the actual fix is to define:

type AnyCollector = Collector<any, any, any>;
type AnyUsageCollector = UsageCollector<any, any, any>;

The currently proposed changes feel to me like they add more constraints to the types (instead of accepting any, they only accept boolean | undefined).
What do you think @restrry?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@mshustov mshustov requested a review from afharo November 24, 2020 17:21
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM!

@mshustov mshustov merged commit d47460d into elastic:master Nov 24, 2020
@mshustov mshustov deleted the attepmpt-to-fix-floating-type-error branch November 24, 2020 17:56
mshustov added a commit to mshustov/kibana that referenced this pull request Nov 24, 2020
* make fetch compatible with CollectorFetchMethod

* use Alejandros suggestion
rylnd added a commit to rylnd/kibana that referenced this pull request Nov 24, 2020
* master: (41 commits)
  [Maps] fix code-owners (elastic#84265)
  [@kbn/utils] Clean target before build (elastic#84253)
  [code coverage] collect for oss integration tests (elastic#83907)
  [APM] Use `asTransactionRate` consistently everywhere (elastic#84213)
  Attempt to fix incremental build error (elastic#84152)
  Unskip "Copy dashboards to space" (elastic#84115)
  Remove expressions.legacy from README (elastic#79681)
  Expression: Add render mode and use it for canvas interactivity (elastic#83559)
  [deb/rpm] Move systemd service to /usr/lib/systemd/system (elastic#83571)
  [Security Solution][Resolver] Allow a configurable entity_id field (elastic#81679)
  [ML] Space permision checks for job deletion (elastic#83871)
  [build] Provide ARM build of RE2 (elastic#84163)
  TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets (elastic#83628)
  [Workplace Search] Initial rendering of Org Sources (elastic#84164)
  update geckodriver to 0.28 (elastic#84085)
  Fix timelion vis escapes single quotes (elastic#84196)
  [Security Solution] Fix incorrect time for dns histogram (elastic#83532)
  [DX] Bump TS version to v4.1 (elastic#83397)
  [Security Solution] Add endpoint policy revision number (elastic#83982)
  [Fleet] Integration Policies List view (elastic#83634)
  ...
mshustov added a commit that referenced this pull request Nov 24, 2020
* make fetch compatible with CollectorFetchMethod

* use Alejandros suggestion
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 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants